Skip to content

🎁 Add killOnServerStop to debug configuration#163779

Merged
roblourens merged 8 commits intomicrosoft:mainfrom
babakks:add-kill-on-server-stop
Nov 17, 2022
Merged

🎁 Add killOnServerStop to debug configuration#163779
roblourens merged 8 commits intomicrosoft:mainfrom
babakks:add-kill-on-server-stop

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.

@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.

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

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

@babakks
Copy link
Contributor Author

babakks commented Nov 17, 2022

There's also a merge conflict here

Merged with the main.

@babakks babakks requested a review from roblourens November 17, 2022 11:04
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 <[email protected]>

* 📜 Add description for `killOnServerStop`

Signed-off-by: Babak K. Shandiz <[email protected]>

* 🔨 Stop created debug session on server stop

Signed-off-by: Babak K. Shandiz <[email protected]>

* 🔨 Push kill listeners into another disposable container

Signed-off-by: Babak K. Shandiz <[email protected]>

* 🐛 Prevent leak when new debug session fails to start

Signed-off-by: Babak K. Shandiz <[email protected]>

* 🔨 Use more verbose name for debug session tracker ID

Signed-off-by: Babak K. Shandiz <[email protected]>

Signed-off-by: Babak K. Shandiz <[email protected]>
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 <[email protected]>

* 📜 Add description for `killOnServerStop`

Signed-off-by: Babak K. Shandiz <[email protected]>

* 🔨 Stop created debug session on server stop

Signed-off-by: Babak K. Shandiz <[email protected]>

* 🔨 Push kill listeners into another disposable container

Signed-off-by: Babak K. Shandiz <[email protected]>

* 🐛 Prevent leak when new debug session fails to start

Signed-off-by: Babak K. Shandiz <[email protected]>

* 🔨 Use more verbose name for debug session tracker ID

Signed-off-by: Babak K. Shandiz <[email protected]>

Signed-off-by: Babak K. Shandiz <[email protected]>

* assign bhayva to getting started

Signed-off-by: Babak K. Shandiz <[email protected]>
Co-authored-by: Babak K. Shandiz <[email protected]>
@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