-
Notifications
You must be signed in to change notification settings - Fork 2
Fix DAP initialization deadlock by making launch request non-blocking #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
+228
to
+232
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While this change correctly fixes the deadlock with The current implementation of 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 To fix this,
This change would be in |
||
|
|
||
| // Wait for initialized event (comes after launch per DAP spec) | ||
| tracing::debug!(timeout_secs = request_timeout.as_secs(), "Waiting for DAP initialized event"); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By switching to
launch_no_waitfor all adapters, this change correctly fixes the deadlock with adapters likelldb-dapanddebugpy. However, this introduces a potential regression for adapters that do respond to thelaunchrequest immediately.Previously,
client.launch()was used for those adapters, which would await the response and propagate any errors. With the switch tolaunch_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_messageto 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.