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

969 enable graceful shutdown with sigint on terminate #1066

Conversation

alex1234567890123456789

I took a stab at issue #969 . This is very important for a project I'm working on where we need proper shutdown behavior to finalize files. It should have no effect by default unless enabled in the configuration. It's only been tested on Linux, as that's the only system I have available, so I have disabled it for non-Linux platforms.

Alex Hoenig added 2 commits February 15, 2024 11:48
- set supportsTerminateRequest to true
- added a handle_terminate method that sends the process SIGINT
…ort (graceful debuggee shutdown)

- capabilities.supportsTerminateRequest is set to false by default unless configured
- enabled SIGINT passthrough to application using "process handle SIGINT -p true -s false" when terminate request support has been enabled
- terminate request support is only enabled on linux due to lack of testing on other platforms
@@ -268,6 +268,7 @@ pub struct AdapterSettings {
pub evaluate_for_hovers: Option<bool>,
pub command_completions: Option<bool>,
pub reproducer: Option<Either<bool, String>>,
pub enable_terminate_request: Option<bool>,
Copy link
Contributor

Choose a reason for hiding this comment

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

these adapter settings are a "custom" message invented by codelldb and only usable in vscode.

Can we avoid adding more? It means that this option is not available in other DAP-compatible clients.

If this behaviour is useful and correct, then why would we not always offer it to all clients?

Choose a reason for hiding this comment

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

I believe this entire MR (and the underlying reported issue) are only applicable to vscode. This new optional shutdown behavior was (somewhat) recently added in vscode. I'm unfamiliar with DAP - does it also have a query for whether a terminate request is supported?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

My point is that if the server supports the terminate request, it should say it does, then it's up to the client to decide whether or not to use it globally not in a spacial way just for one adapter. Users are unlikely to change this setting, nor to really understand what chaining it means. The behviour of the client is consistent among adapters supporting the request, without additional configuration.

Choose a reason for hiding this comment

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

Oh I see... so you're suggesting that this should just be implemented without a configuration item to toggle it? That would certainly be easier, I just added the configuration item (and all the associated plumbing) because I thought this PR would be more likely to be accepted if it didn't change the user experience without opting in. I think it would only be noticed if the debuggee implements a SIGINT handler, in which case it's probably the desired behavior anyways. I'll change that (probably in the morning).

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, ultimately it's a question for @vadimcn to decide - I'm just expressing my preference as a client maintainer.

Choose a reason for hiding this comment

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

Just pushed an update that removes the configuration and enables the behavior on unix platforms.

@vadimcn vadimcn closed this Oct 4, 2024
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