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

refactor: improve and order handling in the Binder #1619

Merged
merged 2 commits into from
Mar 29, 2023

Conversation

connor4312
Copy link
Member

@connor4312 connor4312 commented Mar 29, 2023

Previously the Binder was a little messy. The binder managed the "tree" of debug sessions, but teardown was not handled in a particular order. So, for example, when terminating a Node.js session with a bunch of child processes, the parent process could be terminated and disconnected while its children were nominally still "running" if their async shutdown logic didn't finish yet.

Now that things are nicely ordered, I can fix this one microsoft/vscode#173993

Previously the `Binder` was a little messy. The binder managed the
"tree" of debug sessions, but teardown was not handled in a particular
order. So, for example, when terminating a Node.js session with a bunch
of child processes, the parent process could be terminated and
disconnected while its children were nominally still "running" if their
async shutdown logic didn't finish yet.

Now that things are nicely ordered, I can fix microsoft/vscode#173993
@connor4312 connor4312 marked this pull request as draft March 29, 2023 17:49
@connor4312 connor4312 marked this pull request as ready for review March 29, 2023 17:55
@vscodenpa vscodenpa added this to the March 2023 milestone Mar 29, 2023
connor4312 added a commit to microsoft/vscode that referenced this pull request Mar 29, 2023
If a child session is shut down or shutting down, don't detach it from its parent.

Fixes #173993

Requires microsoft/vscode-js-debug#1619 to work reliably
@connor4312 connor4312 enabled auto-merge (squash) March 29, 2023 18:24
Copy link
Contributor

@rzhao271 rzhao271 left a comment

Choose a reason for hiding this comment

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

LGTM. Ignore my comment earlier.

src/binder.ts Show resolved Hide resolved
@connor4312 connor4312 merged commit ac95032 into main Mar 29, 2023
@connor4312 connor4312 deleted the connor4312/refactor-binder-logic branch March 29, 2023 19:16
connor4312 added a commit to microsoft/vscode that referenced this pull request Mar 29, 2023
If a child session is shut down or shutting down, don't detach it from its parent.

Fixes #173993

Requires microsoft/vscode-js-debug#1619 to work reliably
trbot86 pushed a commit to trbot86/vscode that referenced this pull request Mar 29, 2023
If a child session is shut down or shutting down, don't detach it from its parent.

Fixes microsoft#173993

Requires microsoft/vscode-js-debug#1619 to work reliably
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants