Debugger: Add option to allow disconnect and stop/terminate UI elements

[zobo]

Currently VSCode will present only "stop" or "disconnect" button in a debug session, depending on "request" value in launch.json (launch or attach).

There a re cases where the user would like to choose for them selves if they need a STOP operation (terminating the execution of the debugee) or DISCONNECT (allowing the debugee to run free).

This would probably not require any changes to DAP. There is already a supportTerminateDebuggee flag that could be used to offer both options (disconnect / stop).

Or at least add a vscode command and menu entry like in Visual Studios Detach All.

Let me know if I can provide more context.

Thank you.

[weinand]

This feature request asks for surfacing the optional terminateDebuggee flag in the VS Code debugger UI.
Details:

• if a debug adapter has the supportTerminateDebuggee capability, a new UI should become available for the "Debug: Disconnect" action to control whether "disconnect" just disconnects from the debuggee or terminates the debuggee.
• by default, "disconnect" of a "launch" debug session will terminate the debuggee, and "disconnect" of a "attach" debug session will disconnect. With the new UI it will be possible to change this behavior.
• UI: TBD @isidorn any suggestions?

[isidorn]

This makes sense. So for attach sessions that have a supportTerminateDebuggee we need a new command.
This command could:

1. live in the command palette
2. Be in the context menu of the Call Stack view.
3. Be an alternative command in the toolbar (thus when user holds alt and hovers over disconnect we would show terminate icon)

I would start with these three.

What I would not yet do:

• Have a dropdown disconnect action that offers disconnect vs terminate

Do we also want to do this for launch sessions? If yes, I suggest the above 3 just the other way around (disonnect is the alternative one)

Let me know what you think.

[zobo]

At least from a technical standpoint disconnect is also possible in launch scenario for PHP.
I would agree with 1. 2. and 3. as these introduce new functionality without disrupting the current UI.

[weinand]

@isidorn The "launch" scenario is more important than "attach" because here debug adapters have a more realistic chance to actually being able control whether the debuggee is terminated or not.

In the "attach" case it might be impossible for a debug adapter to terminate the debuggee because it does not have access to the process (because it did not launch the debuggee in the first place).

Other than that I agree with your proposal from above.

[isidorn]

Thanks for feedback. So now we have a plan, I will do 1. 2. and 3. for both launch and attach.

[isidorn]

I have coded up what we have agreed upon. The following gif shows these in practice (in the last part I am holding down the alt key).
@zobo it would be great if you can try this out once it is in Insiders from next week and let us know how it goes.

[zobo]

Hello @isidorn !
I took the last insiders build. I can confirm that I can see the UI elements (alt icon, Debug: Disconnect and Debug: Stop). But I do not see the context menu on the call stack. I figured out that our DAP extension presents stacks somehow different than the internal javascirp:

I can't figure out what is different. When a connection is created we fire a ThreadEvent('started'). I tried a ProcessEvent, but that has no effect, not is it defined in the vscode-debugadapter. Any ideas what I'm doing wrong?

Also, I was under the impression this would be controlled by the supportTerminateDebuggee and expected that this UI change was not available if the supports flag was not set.

Also, I expected to see that the disconnectRequest args should be caring a terminateDebuggee bool. I cant see that... I'm using a launch type configuration.

Thanks!

[isidorn]

@zobo thanks a lot for trying this out.

1. It only appears on the context menu of the debug session. And in the call stack we only show the debug session when there are mutliple debug sessions. js-debug has a child debug session and thus we show them then
2. You are correct the UI change should happen only when supportTerminateDebuggee is there. I will push a fix for this
3. Currently we never send the terminateDebuggee bool. Can you please file a new issue for this, as this feels not specificly related to this issue

[zobo]

Hi @isidorn, thanks for getting back!

1. You are right, I knew that, but did not realize it before.
2. Ok.
3. I'm not completely sure I understand. This started with Unable to trigger DisconnectArguments.terminateDebuggee #116081 where the case was made that VSCode doesn't react to supportTerminateDebuggee. As was acknowledge that VSCode indeed does not use supportTerminateDebuggee and DisconnectArguments.terminateDebuggee, so this feature request was made.
   If the UI is guarded by supportTerminateDebuggee it would only make sense that it also does what is defined in the DAP specification:

 /**
   * The debug adapter supports the 'terminateDebuggee' attribute on the
   * 'disconnect' request.
   */
  supportTerminateDebuggee?: boolean;

From your commits I see that the UI controls whether s.disconnect() or s.terminate() is called. And I just realized that this has to do with supportsTerminateRequest and not supportTerminateDebuggee. So with terminateRequest and not with disconnectRequest/DisconnectArguments.terminateDebuggee. If that's the intended way, 2. point on the list should be to use supportsTerminateRequest instead of supportTerminateDebuggee.

What do you think?
Thanks!

[weinand]

@isidorn please note that this new UI does not change whether a disconnect or a terminate request is issued. It only changes the value of the bool on the disconnect request and it is the responsability of the DA to do the right thing. If the DA doesn't respect the flag, there will be no change in behavior,

The terminate request is completely unrelated to this feature.

[isidorn]

Thanks for correcting me, I was confusing things.
So let me write what should be the behavior:

If the adapter does not supportTerminateDebuggee

UI and disconnect behaves as it does currently and no terminateDebuggee flag is passed

If the adapter does have supportTerminateDebuggee:

1. stop and disconnect are alternative actions via alt. On launch request, stop is default, on attach disconnect is default
2. Every stop UI action actually calls the disconnect with the terminateDebuggee flag true
3. Every disconnect UI action actually calls the disconnect with the terminateDebuggee flag false
4. However if the debug adapter has a supportsTerminateRequest then all stop actions will actually call the terminate as they do today

Please correct me if I misunderstood something, thank you.

[weinand]

@isidorn I believe your proposed behavior is correct, but not crystal clear to me.
Let me try it in a more "code" like way:

Today the UI shows "Stop" for "launch and "Disconnect" for "attach".
Internally the DAP request "disconnect" is used for both cases and the DA "knows" what to do.

With the new UI feature:

If the DA has a falsy supportTerminateDebuggee: nothing changes and DA knows what to do.

If the DA has a true supportTerminateDebuggee:

• VS Code passes the optional boolean terminateDebuggee to the disconnect request with these values:
  • for "launch": true (this makes it explicit to the DA to terminate the debuggee)
  • for "attach": false (this makes it explicit to the DA to leave the debuggee running)
• with the "alt" modifier this behavior can be flipped:
  • for "launch": false (leave the debuggee running)
  • for "attach": true (terminate the debuggee).
• In all cases the action's icon and hover description conveys the semantics of the action (which can be derived from the terminateDebuggee boolean):
  • true: show "red square" icon and "Stop"
  • false: show "detach" icon and "Disconnect"

[isidorn]

Great, we are on the same page. I will code this up now.

[zobo]

This is perhaps a question for DAP project, but what is actually the difference between terminateRequest and disconnectRequest/DisconnectArguments.terminateDebuggee?

First has The ‘terminate’ request is sent from the client to the debug adapter in order to give the debuggee a chance for terminating itself., the other The ‘disconnect’ request is sent from the client to the debug adapter in order to stop debugging. It asks the debug adapter to disconnect from the debuggee and to terminate the debug adapter.

They at least seem redundant?

[weinand]

No, if a DA opts into supporting a terminate request then this and the disconnect request are not redundant.

The disconnect request (with a "true" valued terminateDebuggee) terminates a debuggee forcefully without giving it a chance to clean up properly (or even veto the termination).

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.

To learn how node-debug supports the terminate request please see #54384.

[isidorn]

I have pushed a fix for this as we have agreed upon.
However I could not test it out end to end since both mock-debug and js-debug do not support terminateDebuggee flag.

Also we show the alternative action in the debug toolbar on alt hover even if the debugger does not support terminateDebuggee. This is due to a technical limitation of our menus that we currently do not support alternative actions which are optionally there. We only support alt actions which are always there atm.

[zobo]

Great, I'll test it as soon as it's in insiders build. Thank you!

cc: I think @polinasok was also interested in this...

[weinand]

@isidorn it it possible to disable the alt-actions?
If not, we should consider to show a modal alert and explain that the alt actions are not supported by the debug extension.

[isidorn]

@weinand in theory disabling alt actions should be possible, and I tried just that however in practice it does not work. Since they look as enabled even though clicking on them has no effect. I looked a bit into this issue but could not figure out the exact cause of the problem in the menus. I can investigate more...

[zobo]

I have tested the relevant scenarios for us and it works as expected! Thank you very much!

[isidorn]

Thanks a lot for trying it out, adding verified label per your comment.
If you find additional bugs feel free to open additional issues in this repo.

[polinasok]

Thank you for working on this. I gave this a try as well with insiders.
With a "launch" request, I see Stop by default, which changes to Disconnect if I hold ALT.
If I click Stop, I get {"seq":3,"type":"request","command":"disconnect","arguments":{"terminateDebuggee":true}}
If I click Disconnect, I get {"seq":10,"type":"request","command":"disconnect","arguments":{}}
With "attach", the behavior is reversed.
Looks great!

[weinand]

@isidorn I think the Disconnect from Polina's comment above should be {"seq":10,"type":"request","command":"disconnect","arguments":{"terminateDebuggee":false}}

[isidorn]

@polinasok thanks a lot for trying this out.
I tried to reproduce and I could not, I always see the terminateDebuggee flag set, and that it is set to true for Stop and false for Disconnect as expected.

However the problem might be coming from the fact that we also call disconnect if we see a debuggee terminated. Code pointer.
So I pushed a change that if we see a debuggee terminated we send the terminateDebuggee: false since it is already terminated.

If you can still see the terminateDebuggee flag missing let me know.
@weinand if you think we can improve this process let me know, but I think we need to keep calling disconnect if we see that the debuggee is terminated.

[zobo]

If I may add. In normal usage I don't see this problem, however during tests I see that the args parameter is undefined.

It may be due to https://github.com/xdebug/vscode-php-debug/blob/feat/supportTerminateDebuggee/src/test/adapter.ts#L21 and in specific disconnect test this: https://github.com/xdebug/vscode-php-debug/blob/feat/supportTerminateDebuggee/src/test/adapter.ts#L74

In the second case I explicitly added a parameter object, where before there is none.

I'm thinking about going in a different direction and just use if (args?.terminateDebuggee !== false) { in disconnectRequest instead.

[polinasok]

We have this field marked as omitempy in our Go-to-json spec that we automatically generate from the dap schema. So when we receive a false (empty) value and then log it as json, it gets dropped.