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

feat(dap): send Disconnect if Terminated event received #5532

Merged

Conversation

filipdutescu
Copy link
Contributor

Send a Disconnect DAP request if the Terminated event is received. According to the specification, if the debugging session was started by as launch, the debuggee should be terminated alongside the session. If instead the session was started as attach, it should not be disposed of.

This default behaviour can be overriden if the supportTerminateDebuggee capability is supported by the adapter, through the Disconnect request terminateDebuggee argument, as described in
the specification.

This also implies saving the starting command for a debug sessions, in order to decide which behaviour should be used, as well as validating the capabilities of the adapter, in order to decide what the disconnect should do.

An additional change made is handling of the Exited event, showing a message if the exit code is different than 0, for the user to be aware off the termination failure.

Closes: #4674
Signed-off-by: Filip Dutescu filip.dutescu@gmail.com

Copy link
Contributor Author

@filipdutescu filipdutescu left a comment

Choose a reason for hiding this comment

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

just realised the restart implementation I made is not correct according to the spec. will redo it

helix-view/src/handlers/dap.rs Outdated Show resolved Hide resolved
@pascalkuthe pascalkuthe added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. A-debug-adapter Area: Debug adapter client labels Jan 18, 2023
@filipdutescu
Copy link
Contributor Author

I have re-read the spec and I believe this part of the restart sequence to be correct. What needs to be done in addition is to expose a restart request, if supported, to the user, same as with launch, terminate etc.

Cc: @pascalkuthe could we move it to waiting on review? Many thanks!

@pascalkuthe pascalkuthe added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 18, 2023
@filipdutescu filipdutescu force-pushed the feat/dap_terminate_disconnect branch from cce45ed to d8180a5 Compare January 19, 2023 09:55
@filipdutescu filipdutescu force-pushed the feat/dap_terminate_disconnect branch from d8180a5 to 90e1df2 Compare January 22, 2023 09:29
filipdutescu added a commit to filipdutescu/helix that referenced this pull request Jan 23, 2023
Add a restart debug session command, which would issue a
[Restart Request][1], if the debugger supports it and a session is
running. It uses the same arguments and requests used to start the
initial session, when recreating it.

It builds upon helix-editor#5532, making use of the changes to the termination
workflow of a session.

[1]: https://microsoft.github.io/debug-adapter-protocol/specification#Requests_Restart

Closes: helix-editor#5594
Signed-off-by: Filip Dutescu <filip.dutescu@gmail.com>
helix-dap/src/client.rs Show resolved Hide resolved
@pascalkuthe pascalkuthe added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 1, 2023
@filipdutescu filipdutescu force-pushed the feat/dap_terminate_disconnect branch from 90e1df2 to bde318b Compare February 1, 2023 20:02
@filipdutescu filipdutescu requested a review from archseer February 1, 2023 20:02
@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 1, 2023
helix-dap/src/client.rs Outdated Show resolved Hide resolved
helix-dap/src/client.rs Outdated Show resolved Hide resolved
helix-view/src/handlers/dap.rs Show resolved Hide resolved
helix-view/src/handlers/dap.rs Outdated Show resolved Hide resolved
helix-view/src/handlers/dap.rs Outdated Show resolved Hide resolved
@filipdutescu filipdutescu force-pushed the feat/dap_terminate_disconnect branch from bde318b to 30462ff Compare February 4, 2023 21:59
Send a `Disconnect` DAP request if the `Terminated` event is received.
According to the specification, if the debugging session was started by
as `launch`, the debuggee should be terminated alongside the session. If
instead the session was started as `attach`, it should not be disposed of.

This default behaviour can be overriden if the `supportTerminateDebuggee`
capability is supported by the adapter, through the `Disconnect` request
`terminateDebuggee` argument, as described in
[the specification][discon-spec].

This also implies saving the starting command for a debug sessions, in
order to decide which behaviour should be used, as well as validating the
capabilities of the adapter, in order to decide what the disconnect should
do.

An additional change made is handling of the `Exited` event, showing a
message if the exit code is different than `0`, for the user to be aware
off the termination failure.

[discon-spec]: https://microsoft.github.io/debug-adapter-protocol/specification#Requests_Disconnect

Closes: helix-editor#4674
Signed-off-by: Filip Dutescu <filip.dutescu@gmail.com>
@filipdutescu filipdutescu requested a review from archseer February 4, 2023 22:00
@filipdutescu
Copy link
Contributor Author

@archseer implemented your comments, thanks very much for the patience and advice, I have learnt a lot and hopefully improved the way I write Rust. I also rebased the PR and squashed the commits, to make it a bit cleaner, since there are not that many modifications (hopefully) as to make it a chore to revisit the changes.

Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

Sorry it took me a while to get back to this! LGTM!

@archseer archseer merged commit e3765ac into helix-editor:master Feb 20, 2023
@filipdutescu
Copy link
Contributor Author

No worries, thanks for reviewing and merging it!

filipdutescu added a commit to filipdutescu/helix that referenced this pull request Feb 20, 2023
Add a restart debug session command, which would issue a
[Restart Request][1], if the debugger supports it and a session is
running. It uses the same arguments and requests used to start the
initial session, when recreating it.

It builds upon helix-editor#5532, making use of the changes to the termination
workflow of a session.

[1]: https://microsoft.github.io/debug-adapter-protocol/specification#Requests_Restart

Closes: helix-editor#5594
Signed-off-by: Filip Dutescu <filip.dutescu@gmail.com>
archseer pushed a commit that referenced this pull request Mar 6, 2023
Add a restart debug session command, which would issue a
[Restart Request][1], if the debugger supports it and a session is
running. It uses the same arguments and requests used to start the
initial session, when recreating it.

It builds upon #5532, making use of the changes to the termination
workflow of a session.

[1]: https://microsoft.github.io/debug-adapter-protocol/specification#Requests_Restart

Closes: #5594

Signed-off-by: Filip Dutescu <filip.dutescu@gmail.com>
sagnibak pushed a commit to sagnibak/helix that referenced this pull request Mar 21, 2023
Add a restart debug session command, which would issue a
[Restart Request][1], if the debugger supports it and a session is
running. It uses the same arguments and requests used to start the
initial session, when recreating it.

It builds upon helix-editor#5532, making use of the changes to the termination
workflow of a session.

[1]: https://microsoft.github.io/debug-adapter-protocol/specification#Requests_Restart

Closes: helix-editor#5594

Signed-off-by: Filip Dutescu <filip.dutescu@gmail.com>
wes-adams pushed a commit to wes-adams/helix that referenced this pull request Jul 4, 2023
…#5532)

Send a `Disconnect` DAP request if the `Terminated` event is received.
According to the specification, if the debugging session was started by
as `launch`, the debuggee should be terminated alongside the session. If
instead the session was started as `attach`, it should not be disposed of.

This default behaviour can be overriden if the `supportTerminateDebuggee`
capability is supported by the adapter, through the `Disconnect` request
`terminateDebuggee` argument, as described in
[the specification][discon-spec].

This also implies saving the starting command for a debug sessions, in
order to decide which behaviour should be used, as well as validating the
capabilities of the adapter, in order to decide what the disconnect should
do.

An additional change made is handling of the `Exited` event, showing a
message if the exit code is different than `0`, for the user to be aware
off the termination failure.

[discon-spec]: https://microsoft.github.io/debug-adapter-protocol/specification#Requests_Disconnect

Closes: helix-editor#4674

Signed-off-by: Filip Dutescu <filip.dutescu@gmail.com>
wes-adams pushed a commit to wes-adams/helix that referenced this pull request Jul 4, 2023
Add a restart debug session command, which would issue a
[Restart Request][1], if the debugger supports it and a session is
running. It uses the same arguments and requests used to start the
initial session, when recreating it.

It builds upon helix-editor#5532, making use of the changes to the termination
workflow of a session.

[1]: https://microsoft.github.io/debug-adapter-protocol/specification#Requests_Restart

Closes: helix-editor#5594

Signed-off-by: Filip Dutescu <filip.dutescu@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-debug-adapter Area: Debug adapter client S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DAP should react to Terminated event
4 participants