service/dap: always continue on disconnect from multi-client server?

[polinasok]

Terminal client offers a special flag exit -c to quit the client and resume execution. Vscode UI doesn't offer nuanced disconnect options (https://github.com/microsoft/vscode/issues/134412). Instead a user must first click Continue and then Disconnect to continue on disconnect. If the process is already running when disconnect is requested, it will keep running. If the server is halted, it will stay halted.

A while back the vscode-go legacy adapter was updated to always ask dlv to continue when a remote client disconnected.

Should we do the same in the dap server/adapter? This is likely the desired shortcut for many remote users using multi-client mode (e.g. via CloudCode), but it would make it impossible to halt on disconnect.

Another option is to control this with a boolean attach request flag - .e.g. continueOnDisconnect.

[polinasok]

cc @quoctruong cc @briandealwis

[quoctruong]

Can we have this option on a flag? From past feedback from users, they tend to want their app to be running even after the disconnect. However, this is not a blocker for us.

[polinasok]

Sounds reasonable. Thanks.

[polinasok]

Spoke with @quoctruong offline. It appears that this feature is becoming less critical for CloudCode if necessary at all. CloudCode is starting to provide container setup for the user, so the sessions now consist of launching everything from scratch (as opposed to attaching to an externally created container with dlv running). Thus when users disconnect, everything will be stopped altogether. CloudCode is still collecting user feedback on these changes. And we are yet to receive feedback on remote via DAP after enabling it in vscode-go recently. I am going to keep this open for now. But if we don't hear any explicit user requests in the foreseeable future, I suggest we don't do this. Adding a launch option is easy. Deprecating it is hard. Let's only add what we definitely know our users need, especially when there is a workaround available (explicitly clicking Continue before disconnecting).

[polinasok]

@briandealwis

[briandealwis]

IMHO we should continue on disconnect. In Cloud Code we create 1+N launches: 1 for the main launch, and then a programmatically created launch for each container. For the main launch, the default action is the stop (square). But for the container launches, the default is disconnect. When we stop at a breakpoint, the stack frame is selected within the container launch, and so the default is disconnect. (If I hold alt it turns to a stop.) If I click on disconnect, the launch is removed but my app appears to hang, which feels odd. And because the launch is removed, I have no other actions available to me other than to stop or relaunch the main launch.

[polinasok]

@briandealwis Do people disconnect and reconnect to these container launches?

[briandealwis]

We don’t offer any way to do reattach that I can see.

[polinasok]

Did you use dlv attach or dlv exec/debug to hook into these container launches? Do you want them to keep running after you disconnect or do you want to stop them? The reason why it hangs is because you still have multi-use debugger attached to the process and this debugger is paused on a breakpoint. Probably because you used --accept-multiclient, so you could take advantage of --continue. (We really just need to make continue work on its own.)

[briandealwis]

We use dlv exec —headless —continue —multiclient. We use continue as blocking on startup can lead to Kubernetes apps falling over as readiness and liveness probes will timeout. Multiclient as disconnects do happen and it’s useful for Cloud Code to be able to reattach.

IMHO users would expect disconnecting should resume execution. They do have the option to choose stop to terminate the container.

[polinasok]

if we make resume the default behavior disconnect behavior, users will not have the option to disconnect while paused and then reconnect to the session at the same spot. Ideally they should have both options at their disposal. Anticipating this issue, I got vscode/dap folks to add this as a disconnect flag, but it is still not surfaced by the vscode UI. We can try to add that ourselves.

[hyangah]

IMHO users would expect disconnecting should resume execution. They do have the option to choose stop to terminate the container.

I also think that the default behavior should be to resume execution when getting disconnected (either because user asked to disconnect or because of some underlying connection issues).

users will not have the option to disconnect while paused and then reconnect to the session at the same spot.

Why should this be done through UI instead of a launch setting? Especially when multiple debug sessions are involved, checking every session and clicking/choosing non-default is not very pleasant. I thought this - disconnect while paused - is a rare use case; I don't remember if there were many complaints when the legacy debug adapter changed its behavior around it. If my assumption is true, isn't it too much to ask those users who really want to suspend their remote target to configure their launch setting?

[polinasok]

It would be better to just submit a patch to vscode and enable SuspendDebuggee support than add our own launch setting (which would also be sticky unless we also add it to dlv config). The resume behavior would be the default and suspend would be an option to choose on demand. That's why we made the request to upgrade the protocol.

Quick question about accidental disconnects as opposed to those requested by an explicit user click. Let's say a user is at a breakpoint and inspecting the stacktrace, variables, etc. Then connection is lost and the debug session is relaunched. Why would they prefer that things resumed in that case, making it impossible to go back to where they left off?

[briandealwis]

Can delve detect the difference between a drop and an explicit disconnect?

[hyangah]

@polinasok

It would be better to just submit a patch to vscode and enable SuspendDebuggee support than add our own launch setting (which would also be sticky unless we also add it to dlv config). The resume behavior would be the default and suspend would be an option to choose on demand. That's why we made the request to upgrade the protocol.

What's the "SuspendDebugee" support in VS Code like? I don't see UX/UI discussion mature in the issue tracker. I already found switching manually between terminate & disconnect is not as scalable as I like. If VSCode team thinks this is a frequently requested feature that justifies added complexity to their UI, that's great. EDIT: They need to decide on keyboard shortkey + icon? Or do they think about something different?

Then connection is lost and the debug session is relaunched. Why would they prefer that things resumed in that case, making it impossible to go back to where they left off?

Good point. Cases I thought about was a combined launch configuration -

A main application that's locally launched to send requests to N servers that are attached. The debugging session terminates due to connection issue. Maybe the main application terminates. Now I restart a new debug session with the same launch configuration. The main application is relaunched, and I now need to go through N servers and resume some of the N servers that were left suspended, before the main application task doesn't timeout.

But this is hypothetical and maybe the demand is low.

@briandealwis

Can delve detect the difference between a drop and an explicit disconnect?

I think it can potentially utilize the existence of terminate or disconnect message as a sign.

[polinasok]

Can delve detect the difference between a drop and an explicit disconnect?

I think so. An explicit user action causess the editor to send a request to the server. If connection fails, the server just gets a client connection error when reading the next request.

[polinasok]

What's the "SuspendDebugee" support in VS Code like? I don't see UX/UI discussion mature in the issue tracker. EDIT: They need to decide on keyboard shortkey + icon? Or do they think about something different?

The discussion just got restarted. https://github.com/microsoft/vscode/issues/134412 Let's see where it takes us in the next couple of days. Agree that the existing double toggle is a bit clunky and doesn't scale. It would not hurt to have relevant options to end the session side-by-side (stop only for launch, stop and disconnect+resume for local attach and stop, disconnect+resume and disconnect+suspend for remote attach).