Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 5 additions & 11 deletions src/daemon/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -225,17 +225,11 @@ impl DebugSession {
"Sending DAP launch request"
);

// For Python/debugpy, we need to use non-blocking launch because debugpy
// doesn't respond to launch until after configurationDone is sent.
// We send launch, wait for initialized, send configurationDone, then
// the launch response arrives.
if is_python {
client.launch_no_wait(launch_args).await?;
tracing::debug!("DAP launch request sent (no-wait mode for Python)");
} else {
client.launch(launch_args).await?;
tracing::debug!("DAP launch request successful");
}
// The DAP specification allows adapters to defer the launch response
// until after configurationDone is received. To prevent deadlocks with
// strict adapters (like debugpy and lldb-dap), we use a non-blocking launch.
client.launch_no_wait(launch_args).await?;
tracing::debug!("DAP launch request sent (no-wait mode)");
Comment on lines +231 to +232

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

By switching to launch_no_wait for all adapters, this change correctly fixes the deadlock with adapters like lldb-dap and debugpy. However, this introduces a potential regression for adapters that do respond to the launch request immediately.

Previously, client.launch() was used for those adapters, which would await the response and propagate any errors. With the switch to launch_no_wait, the response is handled by a background task without a registered waiter. This means if an immediate launch response indicates a failure, it will be treated as an 'unknown' response. The error from the adapter will be lost, and the session will proceed as if the launch was successful, potentially leading to confusing failures later.

To mitigate this, consider enhancing DapClient::process_message to more robustly handle unexpected error responses. For instance, if an un-awaited response indicates failure (success: false), it could be logged as an error with its message, rather than just a warning about an unknown sequence number. This would make launch failures from any adapter more visible.

Comment on lines +228 to +232

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

While this change correctly fixes the deadlock with lldb-dap and other strict adapters by using a non-blocking launch universally, it introduces a potential issue for adapters that respond to the launch request immediately.

The current implementation of client.launch_no_wait (in src/dap/client.rs) uses send_request, which does not register a response handler in the pending map. When an adapter that responds immediately (like gdb) sends its launch response, the background reader task finds no corresponding handler and logs a warning: Received response for unknown request seq {}.

This means that if the launch fails for such an adapter, the error response will be ignored, and the session will likely time out later when waiting for the initialized event, masking the true root cause of the failure. This contradicts the PR description's claim that responses are 'handled cleanly'.

To fix this, launch_no_wait should be modified to handle responses asynchronously. A possible approach:

  1. In launch_no_wait, register a oneshot channel in the pending map for the launch request's sequence number before sending it.
  2. Spawn a new, non-blocking tokio task that awaits the response on the oneshot receiver.
  3. Inside this task, check if the response was successful. If not, log the error using tracing::error!. This ensures launch failures are captured without re-introducing the deadlock.

This change would be in src/dap/client.rs and would make the overall solution more robust.


// Wait for initialized event (comes after launch per DAP spec)
tracing::debug!(timeout_secs = request_timeout.as_secs(), "Waiting for DAP initialized event");
Expand Down
48 changes: 47 additions & 1 deletion src/dap/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -706,7 +706,53 @@ impl DapClient {
/// configurationDone is sent. This sends the launch request but doesn't
/// wait for the response.
pub async fn launch_no_wait(&mut self, args: LaunchArguments) -> Result<i64> {
self.send_request("launch", Some(serde_json::to_value(&args)?)).await
let seq = self.next_seq();
let (tx, rx) = tokio::sync::oneshot::channel();
{
let mut pending_guard = self.pending.lock().await;
pending_guard.insert(seq, tx);
}

let request = serde_json::json!({
"seq": seq,
"type": "request",
"command": "launch",
"arguments": serde_json::to_value(&args)?
});

let json = serde_json::to_string(&request)?;
tracing::trace!("DAP >>> {}", json);

if let Err(e) = codec::write_message(&mut self.writer, &json).await {
let mut pending_guard = self.pending.lock().await;
pending_guard.remove(&seq);
return Err(e);
}

// Spawn a background task to await the response and log any failures
tokio::spawn(async move {
if let Ok(inner_result) = rx.await {
match inner_result {
Ok(response) => {
if !response.success {
tracing::error!(
"Launch request failed: {}",
response.message.unwrap_or_else(|| "Unknown error".to_string())
);
} else {
tracing::debug!("Launch request succeeded (async)");
}
}
Err(e) => {
tracing::error!("Launch request resulted in an error: {}", e);
}
}
} else {
tracing::debug!("Launch response channel closed before receiving a reply");
}
});

Ok(seq)
}

/// Attach to a running process
Expand Down