DAP - Terminate Request is not sent for "Attach" Configurations

[ericdrobinson]

Details

Does this issue occur when all extensions are disabled?: N/A

• VS Code Version: 1.64.2
• OS Version: macOS 12.2.1

Steps to Reproduce

1. Initialize a Debug Session with supportsTerminateRequest.
2. Create an "attach" configuration.
3. Start a debug session using the "attach" configuration.
4. Press Alt/Option to show the "Stop" button in the Debugger UI.
5. Click the "Stop" button.

Expectation

If I implement a Debug Adapter that supports the Terminate Request so that I can handle the two-phase "Graceful Termination", then I expect that functionality to work in both "Launch" and "Attach" configurations. If the user presses the "Stop" button, then this internally indicates "terminate debuggee". This is indicated by the fact that the terminateDebuggee Disconnect Request argument is true (if support is enabled) when the user presses the "Stop" button. This applies to both "Attach" and "Launch" configurations.

Reality

The Terminate Request is never called for Debug Sessions started with "Attach" configurations, even when the user clicks the "Stop" button to trigger termination.

The DAP specification does not suggest that the Terminate Request is limited to work only with "Launch" configurations.

History

The following appear to be related:

• Support DAP's terminate request when ending a debug session #54384
• don't send 'terminate' request for 'attach' configs #54420
• Debugger: Add option to allow disconnect and stop/terminate UI elements #116731

It appears that this "rule" was implemented before the Alt/Option feature that allows users to switch between Disconnect/Stop buttons in the UI. When this new feature was under discussion, @weinand wrote the following:

The terminate request can be used to trigger a "soft" termination of a debuggee, which the debuggee can intercept to do some cleanup work. While doing this the debuggee can still be debugged and the debuggee can even decide to ignore the terminate and continue execution.

My expectation as a Debug Adapter implementer is that if I support the Terminate Request, then whenever the user initiates termination (e.g. via the Stop button, regardless of how the Debug Session was started) I should receive the Terminate Request. The check for whether to call Terminate Request should no longer check for "Launch" vs "Attach" but, rather, "Terminate" vs "Disconnect".

Perhaps this was simply an oversight during the initial implementation as @isidorn wrote the following:

However if the debug adapter has a supportsTerminateRequest then all stop actions will actually call the terminate as they do today

Which is true, they do - at least the ones that existed prior to this change. The newly added stop action (Alt/Option to enable Stop with "Attach" Debug Sessions) appears to have slipped through the cracks as it is not currently included in the "all stop actions".

[weinand]

VS Code has probably a bug here, but the issue is a bit theoretical because I do not really see a way how the "Graceful Termination" of a debuggee can be implemented for the "attach" case, where the debuggee is not under the control of the debug adapter.

[ericdrobinson]

because I do not really see a way how the "Graceful Termination" of a debuggee can be implemented for the "attach" case, where the debuggee is not under the control of the debug adapter.

@weinand I don't understand this statement. Perhaps you can provide an example with a specific Debug Adapter that you have in mind?

Perhaps there are some Debug Adapter implementations where once connected to a process running a debuggee script/program the Debug Adapter cannot trigger some SIGINT and SIGKILL combination? But there are definitely implementations where that can be done - or at least something like it could be implemented to better align the "attach" and "launch" termination cases. The Debug Adapter I'm working on is one such where the debug adapter has the ability to terminate (halt) a running script (the debuggee) at any time, whether "attached" or "launched". We may even benefit from a "Graceful Termination" approach (the distinction between the launch configurations in our case is somewhat semantic). Having there be a distinction between "attach" and "launch" for termination here is just confusing and unnecessary. Our users would expect that if they started with "attach" and switched to the "Stop" UI that it would work the same way as a "launch" that wasn't switched to the "Disconnect" UI.

[ericdrobinson]

@weinand I have just reread your last comment and I think perhaps you may have misunderstood the request.

I am not asking for "Graceful Disconnect" here. I am asking that when the user holds Alt/Option to reveal the "Stop" button (which implies "terminate" to the Debug Adapter), that an extension that has registered for the TerminateRequest continue to receive that request for "Attach" sessions as well.

So when you say:

I do not really see a way how the "Graceful Termination" of a debuggee can be implemented for the "attach" case

I'm not suggesting that you add a two-phase disconnect (as #196) does. I'm requesting that VS Code's DebugSession.terminate() function:

vscode/src/vs/workbench/contrib/debug/browser/debugSession.ts

Lines 325 to 348 in 188b1a5

	/**
	* terminate the current debug adapter session
	*/
	async terminate(restart = false): Promise<void> {
	if (!this.raw) {
	// Adapter went down but it did not send a 'terminated' event, simulate like the event has been sent
	this.onDidExitAdapter();
	}
	this.cancelAllRequests();
	if (this._options.lifecycleManagedByParent && this.parentSession) {
	await this.parentSession.terminate(restart);
	} else if (this.raw) {
	if (this.raw.capabilities.supportsTerminateRequest && this._configuration.resolved.request === 'launch') {
	await this.raw.terminate(restart);
	} else {
	await this.raw.disconnect({ restart, terminateDebuggee: true });
	}
	}
	if (!restart) {
	this._options.compoundRoot?.sessionStopped();
	}
	}

replace the following line:

vscode/src/vs/workbench/contrib/debug/browser/debugSession.ts

Line 338 in 188b1a5

if (this.raw.capabilities.supportsTerminateRequest && this._configuration.resolved.request === 'launch') {

with this:

if (this.raw.capabilities.supportsTerminateRequest)

The DebugSession.terminate() method itself should only be called when the user presses the Stop button (⏹️) in the Debug Toolbar UI. Users can do that for Attach configurations by holding Alt/Option while hovering over the Disconnect button. This ability of users to request "Termination" for an "Attach" Debug Session was added in #116731. That was well after the above sanity check for launch was added.

This request is simply about fixing "Debuggee Termination (initiated by the Stop button) while in Attach-type Debug Sessions". It has nothing to do with Disconnect flow.

[weinand]

@ericdrobinson you said in the initial comment:

If I implement a Debug Adapter that supports the Terminate Request so that I can handle the two-phase "Graceful Termination", then I expect that functionality to work in both "Launch" and "Attach" configurations.

I have to disagree.
The fundamental difference between "launch" and "attach" is this:

• in the "launch" case the VS Code debugger "owns" the debuggee's lifecycle because it launches the debuggee and is thus responsible for terminating the debuggee when the debug session is ended via the "Stop" command.
• in the "attach" case the debuggee's lifecycle is owned by some external entity (e.g. a server or the user launcing the debuggee from the command line) and the debugger is not responsible for terminating the debuggee when a debug session is ended via the "Stop" command.

So with the default behavior of the "Stop" command for "attach" configurations the terminate request should not be called. But when using the Alt-key to flip the behavior of the "Stop" command for "attach" to "terminate", then the terminate request must be called.

[ericdrobinson]

@weinand I'm not sure what to say other than I'm a consumer of the DAP and I have an implementation that does not match your expectations.

The fundamental difference between "launch" and "attach" is this:

• in the "launch" case the VS Code debugger "owns" the debuggee's lifecycle because it launches the debuggee and is thus responsible for terminating the debuggee when the debug session is ended via the "Stop" command.
• in the "attach" case the debuggee's lifecycle is owned by some external entity (e.g. a server or the user launcing the debuggee from the command line) and the debugger is not responsible for terminating the debuggee when a debug session is ended via the "Stop" command.

I guess I don't care who's "responsibility" it is when I have the ability in the debuggee protocol that I'm working with to request that the debuggee terminate itself even when I attached to the running process. Sure, I didn't start the process, but I do still have the ability to request that it shut itself down. What is so sacred about "who started a process" here?

Additionally, if you are so darn adamant about this:

the debugger is not responsible for terminating the debuggee when a debug session is ended via the "Stop" command.

Why do you allow the user to switch the UI from "Disconnect" to "Stop" in the UI of an an attached adapter and send the terminateDebuggee flag in those cases?

Note: Please understand that I am maintaining a Debugger Extension that makes excellent use of this feature/capability in a way that enables great workflow for the end user. Removing the terminateDebuggee flag for the Attach case as it currently works in order to better align operation with your description above would actually be detrimental to us.

[weinand]

The fundamental difference between "launch" and "attach" configurations exist in DAP since its inception 6 years ago and 50+ debuggers are using this. Discussions about the history might be interesting for some, but they cannot result in changes to the history (because that would be breaking). So we will have to live with semantics that sometimes does not match your (or any other debug extension author's) expectations.

To mitigate these mismatch problems we are always trying to introduce new features that help those debug extension authors that are limited by bad semantics. One example is the terminateDebuggee argument for the disconnect request. I don't understand why you think that terminateDebuggee will be removed? I never said this anywhere (and you should know by now that nothing can be ever removed from DAP anyway ;-)

This bug report will fix the problem that you describe in your initial comment:
if the terminateDebuggee argument is true then terminate must be called in the "attach" case as well.

[ericdrobinson]

This bug report will fix the problem that you describe in your initial comment:
if the terminateDebuggee argument is true then terminate must be called in the "attach" case as well.

Ahh, got it. Somewhere I guess we started talking past each other because this is simply what I've been advocating for :)

I don't understand why you think that terminateDebuggee will be removed? I never said this anywhere (and you should know by now that nothing can be ever removed from DAP anyway ;-)

Ahh, yes, but that doesn't stop you from changing how the DAP implementation in VSCode works (provided it doesn't contradict anything explicitly spelled out in the protocol...) ;-)

Though... I admit I didn't check the wording of the relevant content in the docs exactly. Apologies for the confusion!