Allow a debuggee to be disconnected gracefully

[yannickowow]

Hello,
I would like to know it if would make sense to add the ability to disconnect (on attach mode) gracefully using the same behaviour as terminateRequest.

My first guess would be if a debuggee has supportsTerminateRequest to true, the first detach command or stop command will trigger terminateRequest (with an optional flag) instead of disconnectRequest. When detaching, a debuggee can emit outputs and would be ignored if a client (such as VS Code) shutdowns immediately a debug session.

Regards.

[weinand]

@yannickowow yes, I think this feature request makes sense (and it was reported by @ericdrobinson too)

[ericdrobinson]

Would this make sense to partially implement as a new feature? The supportsTerminateRequest is sort of specific in its implementation at this point to "termination" as opposed to "disconnection". Perhaps it makes sense to add a supportsGracefulDisconnect or the like?

I believe the issue I reported that @weinand is referring to is this one, but it is VS Code-specific and is specifically about VS Code's implementation of supportsTerminateRequest being ignored when a "Stop" (termination) signal is sent by the user during an "Attach" Debug Session. In recent versions of VS Code, you can hold the Alt/Option key while hovering over the Debug Toolbar to switch between Disconnect/Stop buttons. The button state flip is opposite for Launch and Attach configs, but in both cases both the "Disconnect" and "Stop" buttons are available. My request there is to simply ensure that the "Stop" flow in VS Code works the same for an Attached Debug Session as it does for a Launched Debug Session.

In this case, it seems that the request is to allow a "Graceful Disconnect" option, whereby there would be a two-stage disconnect request. I guess supportsTerminateRequest could be overloaded to take on this meaning, but if that were the case I would recommend deprecating the supportsTerminateRequest capability in favor of something like supportsGracefulDebuggeeStop or something that isn't specific to the TerminateRequest. The supportsTerminateRequest is already confusing enough (#249).

[weinand]

@yannickowow DAP's terminate request is completely orthogonal (independent) to the disconnect request.

A hypothetical client could surface a terminate request as a separate "Shut down debuggee gracefully" command (instead of integrating it into the "Stop Debug" command as VS Code does).
Using that command would trigger the "graceful cleanup code" in the debuggee before the debuggee terminates itself.

There are two possible outcomes of the "Shut down debuggee gracefully" command:

• if everything went well, then the debuggee has terminated itself which ends the debug session and there is no need for a subsequent disconnect request (and the discussion about a "graceful disconnect" is moot).
• if the terminate request did not result in the debuggee terminating itself, then the debug session will just continue and the user can now end the debug session by either disconnecting or by disconnecting and terminating the debuggee. So here a "graceful disconnect" is possible but only in the "vetoed" case.

Bottom line: from a DAP perspective "disconnect gracefully" is possible for the "vetoed" case, but it does not make sense for the un-vetoed case because a "terminate" means "terminate" and not just "disconnect".
So there is nothing to be done here.

---

Now back to VS Code's implementation of this part of the spec:

In VS Code calling the terminate request is integrated into the "Stop Debug" command:
if a debug adapter supports the terminate request, the first run of the "Stop Debug" command calls terminate and if that fails ("is vetoed"), a second run of the "Stop Debug" command calls disconnect.

Since the ultimate goal of the terminate request is to let the debuggee terminate itself gracefully, VS Code made the assumption that terminate needs only to be called in the "launch" case where the debug adapter "owns" the lifecycle of the debuggee and is responsible for terminating the debuggee.
In the "attach" case the terminate request is not called, because the debug adapter does not own the debuggee's life cycle and is not responsible for terminating the debuggee.

However, recently we've added a new argument terminateDebuggee to the disconnect request so that clients can better control whether the debug adapter should terminate the debuggee or not.
But this requires that VS Code does not only call the terminate request in the "launch" case but also when the terminateDebuggee argument is true.

This bug is tracked as microsoft/vscode#143991

[yannickowow]

Correct, thanks for your clarifications.

[ericdrobinson]

@weinand I think you may have misinterpreted the initial request. Specifically that request was for:

the ability to disconnect (on attach mode) gracefully using the same behaviour as terminateRequest.

I read this as a feature request to add a supportsDisconnectRequest that aligns with the supportsTerminateRequest functionality, rather than piggybacks on it. The reason that I think you may have misinterpreted this is because of your explanation as to why this should be closed and not supported (emphasis mine):

There are two possible outcomes of the "Shut down debuggee gracefully" command:
...
Bottom line: from a DAP perspective "disconnect gracefully" is possible for the "vetoed" case, but it does not make sense for the un-vetoed case because a "terminate" means "terminate" and not just "disconnect".

In the bolded part you seem to understand the request as something that depends upon the "terminate" request. Based on the section I quoted above, I think this is a mismatch.

The initial request is for a completely parallel flow that would allow a user to attempt to disconnect an attached debuggee "gracefully". For instance, I send a Debuggee a message that says "I'm disconnecting from you" and the debuggee responds with "wait! Error!" then I can't gracefully disconnect (or I need to show a last-minute error). If the user were to hit the disconnect button again, then it would skip to the standard disconnect logic. This would only be the case if some new supportsDisconnectRequest feature were enabled.

You will notice that my understanding has nothing to do with termination. It is simply a way of saying "if we can support "terminating the host gracefully", why not also offer the opportunity to "disconnect from the host gracefully"?

[ericdrobinson]

Since the ultimate goal of the terminate request is to let the debuggee terminate itself gracefully, VS Code made the assumption that terminate needs only to be called in the "launch" case where the debug adapter "owns" the lifecycle of the debuggee and is responsible for terminating the debuggee.

But you are making an assumption here about how all debuggers will work (I'm working on a debugger for which this doesn't make sense, but another interpretation does). You also allow users to click "Stop" instead of "Disconnect" from the Debug Controls when in an "Attach" session. That "Stop" is sent as a request to terminate (in the disconnectRequest() callback). We handle this in our adapter and it is fantastic. What is very weird is that the terminateRequest works from launch requests when I hit that stop button, but not from attach requests. This does not match my expectations as a user.

In the "attach" case the terminate request is not called, because the debug adapter does not own the debuggee's life cycle and is not responsible for terminating the debuggee.

You are making an assumption about how all debuggers/debuggees work. I'm working with a debuggee protocol that allows me to send a signal to terminate at any time, even if the debug adapter was attached. I cannot "gracefully" handle these situations if a user intentionally uses the "Stop" button in an "Attached" adapter session as a result of this decision.

[weinand]

@ericdrobinson since @yannickowow is the author of this request, I'm taking his comment as a confirmation that my interpretation was correct.

For the other concerns please see microsoft/vscode#143991 (comment)

[yannickowow]

On my side yes I assume that your description of this case is correct, especially since you've linked the related issue in VS Code which suits what was the point of this issue. The issue I mainly faced was the way VS Code implements this, since disconnect does not wait for the disconnect response to close the session.

[ericdrobinson]

@yannickowow There are conflicts between your statement and what @weinand is describing, I think.

You use the term "disconnect" twice in this sentence:

The issue I mainly faced was the way VS Code implements this, since disconnect does not wait for the disconnect response to close the session.

And I interpret that as:

... since [the disconnect button] does not wait for the disconnectRequest() response to close the session.

There are two buttons in VSCode's UI: Disconnect and Stop. The terminateRequest() is only triggered when the Stop button is clicked. None of the "graceful" two-phase logic works unless you have the supportsTerminateRequest option set and the user clicks the Stop button (currently this is only in launch request types, the bug I linked is about ensuring that the same functionality works in attach request types).

There are two other reasons that I think you may be asking for something different:

1. The title of your Issue is asking for Graceful Disconnect. The bug I linked will still only enable Graceful Termination in VSCode.

2. Your initial post contains the following:

   My first guess would be if a debuggee has supportsTerminateRequest to true, the first detach command or stop command will trigger terminateRequest (with an optional flag) instead of disconnectRequest.

   Here you state "... the first detach command or stop command...".

This bug has nothing to do with enabling a Graceful Disconnect when the user hits the "detach command" (I assume that this is the "Disconnect" button in VSCode's UI). It is only about ensuring that the terminateRequest() flow is called when the user clicks the "Stop" button in VSCode's UI during attach requests.

My understanding of this feature request was to add a second "Graceful [Ending]" option to the DAP. This would look like:

1. supportsGracefulTerminationRequest - This is the same as the existing supportsTerminationRequest. The name for this is unfortunately confusing and I believe should be renamed for clarity, with the old one being deprecated.
2. supportsGracefulDisconnectRequest - When the user clicks a "Disconnect" button, a tryDisconnectRequest() function is called. If that returns a successful response, then the debug session is ended. If it is not called (e.g. because a disconnection attempt resulted in a breakpoint being hit) then the debug session continues until the user clicks the "Disconnect" button again, at which point, the disconnection is triggered regardless of success/failure and the debug session will end.

Item 2 above does not currently exist (to my knowledge) and it is what I understand this request to be about.

[yannickowow]

.. since [the disconnect button] does not wait for the disconnectRequest() response to close the session.

Yes, I meant that the disconnect procedure in Visual Studio Code does not wait the disconnectRequest() response to close the entire session. Considering that, I think that the issue is on the client side, not the protocol by itself.

My main concern in Disconnect Gracefully was the ability to correctly display stdout available in the pipe, in the Debug Console. A "graceful" disconnection would mean that the client is aware of the disconnection process, but would not give up directly when the button is clicked. DAP will clean up in the disconnect request, and the disconnectResponse() will give the client the ability to show that it has been correctly done. Client should await the ACK to finish the session.

[ericdrobinson]

🤔
@weinand Does that sound like a protocol-level issue or an implementation-level issue (VSCode) to you?

[weinand]

This is an implementation-level issue.

Please note: the "graceful disconnect" introduced and discussed here is completely unrelated to the "graceful terminate" supported via the DAP terminate request:

• "graceful terminate" supports a feature of the debuggee to shutdown in a controlled manner (as opposed to being forcefully killed). So a developer can use the "graceful terminate" to test and debug the "graceful shutdown" feature of the program being developed.
• "graceful disconnect" was used by @yannickowow to describe "the ability to correctly display stdout available in the pipe, in the Debug Console." This is not a feature of the debuggee, but a feature of the interaction between client, debug adapter, and debugger which is of no interest to the developer. From a developer's perspective this interaction is a "black box": it should just work. It does not make sense to introduce a "Disconnect gracefully" command.

[ericdrobinson]

Understood! Thank you for the clarifications!