Skip to content
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

Prevent race condition on initial breakpoints from DAP #84895

Merged
merged 1 commit into from
Jan 9, 2024

Conversation

rsubtil
Copy link
Contributor

@rsubtil rsubtil commented Nov 14, 2023

When investigating debugging issues being reported on godot-vscode-plugin, I've encountered a race condition regarding initial breakpoints. As far as I can tell, there were no reported issues here on godot and godot-vscode-plugin regarding this.

The DAP implementation assumes all configuration is done before launching the debuggee. Particularly, that breakpoints are set before launching the project. However, that's not the case in, for instance, VSCode, which seems to set breakpoints after requesting DAP to launch a process. Initially I thought this was their issue, but microsoft/vscode#4902, microsoft/vscode#4567 and microsoft/vscode#4902 (comment) seem to suggest we cannot rely on clients to adhere to a specific ordering.

This causes a race condition that happened reliably on a Windows machine: setting a breakpoint on a _ready function did not trigger at all, since this information was not sent to the launched Godot instance on CLI (e.g. --breakpoints res://Button.gd:6), and by the point the Godot editor communicated these breakpoints at runtime, it was too late.

I'm still confused about this whole ordeal and whether the reasoning in the linked comments is actually correct, but one thing we can actually do to adress this is to wait for a configurationDone request that signals the end of configuration before launching the debuggee. This is also the solution implemented for vscode-mock-debug, and so this change essentially defers the launch request to when we receive the configurationDone request.

This comes with the implication that if a DAP client that does not support/send a configurationDone request, then Godot will remain stuck waiting for it. However, this request was added very early on v1.2.x, so I believe this scenario is extremely unlikely.

@rsubtil rsubtil requested a review from a team as a code owner November 14, 2023 15:42
@rsubtil rsubtil force-pushed the fix_dap_race_condition branch from 196247c to 528df34 Compare November 14, 2023 15:43
@AThousandShips AThousandShips added this to the 4.3 milestone Nov 14, 2023
@rsubtil rsubtil force-pushed the fix_dap_race_condition branch from 528df34 to 4853424 Compare November 14, 2023 15:56
@akien-mga akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Nov 14, 2023
@akien-mga akien-mga requested a review from Faless December 5, 2023 13:14
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Neither me nor @Faless are very familiar with this code, but we trust @rsubtil's assessment as the maintainer of DAP.

@akien-mga akien-mga merged commit 4baa634 into godotengine:master Jan 9, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@rsubtil rsubtil deleted the fix_dap_race_condition branch January 9, 2024 17:53
@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.

@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants