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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃巵 Add killOnServerStop to debug configuration #163779

Merged
merged 8 commits into from Nov 17, 2022

Conversation

babakks
Copy link
Contributor

@babakks babakks commented Oct 16, 2022

Fixes #163124

A new configuration parameter, named killOnServerStop, is added. When this parameter is enabled, child debug sessions will be stopped whenever the server (the parent session) stops. The default value for the parameter is false to keep the existing behavior unchanged.

Verification

To verify this PR, use babakks/vscode-verify-kill-on-server-stop which is made for this purpose. Instructions are available in the README.md of the repo.

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
@babakks babakks changed the title Add kill on server stop Add killOnServerStop to debug configuration Oct 16, 2022
@babakks babakks changed the title Add killOnServerStop to debug configuration 馃巵 Add killOnServerStop to debug configuration Oct 16, 2022
@roblourens roblourens added this to the November 2022 milestone Oct 27, 2022
}
});
this.lateDisposables.push(listener); // In case `trackerId` of interest was never caught anyhow.
});
Copy link
Member

Choose a reason for hiding this comment

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

Applicable to here and below:

  • The listener will be leaked if startDebugging returns false
  • Even after the listener resolves, lateDisposables will keep growing indefinitely.

Maybe lateDisposables should be a Set that you can delete disposables from.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. Will push a fix soon.

Copy link
Contributor Author

@babakks babakks Nov 12, 2022

Choose a reason for hiding this comment

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

Made the changes as you suggested. the lateDisposables is a Set now. Also used vscode.CancellationToken for signaling whether to stop listening for events anymore.

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
connor4312
connor4312 previously approved these changes Nov 14, 2022
Copy link
Member

@connor4312 connor4312 left a comment

Choose a reason for hiding this comment

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

looks sane to me, cc @roblourens

return vscode.debug.startDebugging(session.workspaceFolder, {
type,
name: 'Browser Debug',
request: 'launch',
url: uri,
webRoot: session.configuration.serverReadyAction.webRoot || WEB_ROOT
webRoot: session.configuration.serverReadyAction.webRoot || WEB_ROOT,
trackerId,
Copy link
Member

Choose a reason for hiding this comment

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

Please use a more unique name for this. Since it's being added to another extension's launch config, I think it should reflect where it came from and the fact that it's sort of internal, I suggest _debugServerReadySessionId.

Actually it's interesting that startDebugging doesn't return a DebugSession, I sort of expected it would. That would make this easier.

Copy link
Member

Choose a reason for hiding this comment

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

It would indeed. But getting an API change in for that would be awkward, we'd need to duplicate the method 馃槙

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@roblourens
Copy link
Member

There's also a merge conflict here

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
@babakks
Copy link
Contributor Author

babakks commented Nov 17, 2022

There's also a merge conflict here

Merged with the main.

Copy link
Member

@roblourens roblourens left a comment

Choose a reason for hiding this comment

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

Works great, thanks for this!

@roblourens roblourens merged commit 9f56e36 into microsoft:main Nov 17, 2022
lramos15 pushed a commit that referenced this pull request Nov 17, 2022
* 馃巵 Add `killOnServerStop` to schema

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>

* 馃摐 Add description for `killOnServerStop`

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>

* 馃敤 Stop created debug session on server stop

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>

* 馃敤 Push kill listeners into another disposable container

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>

* 馃悰 Prevent leak when new debug session fails to start

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>

* 馃敤 Use more verbose name for debug session tracker ID

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
lramos15 added a commit that referenced this pull request Nov 17, 2022
* Remove i18n causing classifier to fail due to missing label

* 馃巵 Add `killOnServerStop` to debug configuration (#163779)

* 馃巵 Add `killOnServerStop` to schema

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>

* 馃摐 Add description for `killOnServerStop`

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>

* 馃敤 Stop created debug session on server stop

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>

* 馃敤 Push kill listeners into another disposable container

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>

* 馃悰 Prevent leak when new debug session fails to start

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>

* 馃敤 Use more verbose name for debug session tracker ID

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>

* assign bhayva to getting started

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Co-authored-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
@babakks babakks deleted the add-kill-on-server-stop branch November 17, 2022 15:59
@github-actions github-actions bot locked and limited conversation to collaborators Jan 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Actions launched from a server ready action in compound launch configs don't honor the stopAll flag
3 participants