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

Add an option to suspend or keep the target running when disconnecting from a running debugger #177

Closed
polinasok opened this issue Jan 24, 2021 · 11 comments
Assignees
Labels
feature-request Request for new features or functionality
Milestone

Comments

@polinasok
Copy link
Contributor

polinasok commented Jan 24, 2021

#175 is the prerequisite. With Go and dlv, when users launch the debugger on a remote machine, they might want to connect to it for multiple disjoint debug sessions. The end of each session then closes the socket connection, but does not terminate the debugger by default. The debugger continues to run and can either keep the target suspended or running. This option to continue is part of the exit command offered by dlv. This could be an additional option, so users can pick any of the following combinations:

  • terminateDebuggee=true
  • terminateDebuggee=false, terminateDebugger=true
  • terminateDebuggee=false, terminateDebugger=false, suspendDebuggee=false
  • terminateDebuggee=false, terminateDebugger=false, suspendDebuggee=true
@weinand
Copy link
Contributor

weinand commented Mar 18, 2021

@polinasok in the description from above you said

"This option is part of the exit command"

What do you mean by this?
DAP neither has an "exit" request nor does it provide "commands".

But DAP's disconnect request has an optional terminateDebuggee argument which controls whether a debuggee is terminated at the end of a debug session of type "launch".

Is this feature request really asking for a new DAP feature or is it more a client (e.g. VS Code) feature you are looking for?

@polinasok
Copy link
Contributor Author

polinasok commented Mar 18, 2021

I have clarified the original description.
Users of dlv cli have access to certain features that we would like to expose via the adapter. One such feature is to specify on client exit (in DAP terms on disconnect) that while the program stays running and the debugger attached, the execution should stay suspended or should continue. I do not see a way to specify for the debugger to stay running (#175), and I do not see a way for the debugger to stay running+suspended or running+continuing in DAP and in vscode. In cases where these options would be of value, the vscode-go adapter just leaves the server running and issues a continue prior to disconnecting the session. A user doesn't have an option to impact this default behavior. It seemed to me that before that option is supported in the editor, it should be supported in the protocol as well.

@weinand
Copy link
Contributor

weinand commented Mar 18, 2021

Currently DAP does not surface "debugger" in any way (and consequently the VS Code client knows nothing about debuggers either). DAP only surfaces "debug sessions" that are started via "launch" or "attach" and are ended via "disconnect".

Until now debuggers are an implementation detail of debug adapters. They don't "leak" outside of the DA.
If a debugger needs to stay alive across debug sessions, then there is no specific support for this in DAP for this.

However, that does not necessarily mean that it is impossible to have a "long running debugger" with DAP.
It just means that support for a "long running debugger" must be implemented outside of DAP.

For example from a VS Code perspective, it would be easily possible to manage a "long running debugger" in an extension (which is long running too), and reuse that debugger whenever VS Code starts a new debug session.

If I understand your request correctly, you are asking for making "debuggers" explicit in the protocol and then adding UI so that the user can manager them.

@polinasok Is this a correct assessment of your feature request?

@polinasok
Copy link
Contributor Author

This goes back to our offline discussion a while back about the meaning of "long running debugger" and "debug session" in DAP vs delve. DAP has a certain view of the debugging world where each debugging session starts with launch/attach request from a client and ends with disconnect/detach. In that case, it is an irrelevant implementation detail if the DA or the debugger or the DA-debugger-in-one survive past the end of the debug session and if another Start Debugging session reuses a long-running DA or debugger or starts up a new one.

But in the world of Go debugging, especially when doing remote debugging, users can also do the following:

  • start delve server and have it launch a new process OR attach to an already running process
  • since delve is already attached to a process, we can say that debugging has started
  • first client connects to the server and joins the pending debug session
  • first client does some debugging
  • first client exits, but specifies that the server keeps running and that the process should stay suspended or continue
  • the breakpoints set by the first client carry on
  • second client connects to the server and joins the pending debug session
  • if it was running, it suspends on entry; otherwise it picks up at whatever suspended point the process is at
  • second client can see the breakpoints from the first client
  • second client exits
  • ...
  • last client exits and asks for the server to be terminated this time and optionally the process as well

This is what disconnecting looks like with just the command-line client, but users actually connect and disconnect with dlv cli, vscode and even GoLand to the same debug session (e.g. because they prefer features in one vs the other for some things, but not others).

Server attaches to a process and does not stop it on entry:

$ dlv attach 54813  --headless --accept-multiclient --continue
API server listening at: 127.0.0.1:53825

Client A suspends the process when connecting:

$ dlv connect :53825
Type 'help' for list of commands.
(dlv) exit
Would you like to kill the headless instance? [Y/n] n

Process stays suspended. Debugger is still alive and attached.
Client B picks up where Client A left off:

$ dlv connect :53825
Type 'help' for list of commands.
(dlv) exit -c

Process continues on exit. Debugger is still alive and attached.
Client C connects and then shuts everything down when exiting.

$ dlv connect :53825
Type 'help' for list of commands.
(dlv) exit
Would you like to kill the headless instance? [Y/n] y
Would you like to kill the process? [Y/n] y

Sometimes these clients even connect at the same time (and then they have to be very careful not to block each other, but that's another story).

Given that DAP is a debug protocol and debugging cannot happen without a debugger, I don't think having some debugger-centric settings in the protocol is a big stretch. There are already mechanisms for adapters on behalf of the debuggers to specify if they support rewinding or some other feature that might or might not exist in all debuggers. The ability to survive past the end of the debug session to accept more client connections could be just another capability that a debugger might support that would enable more things for the user to specify on disconnect. I have no idea if other debuggers would ever want to take advantage of this. If not, then I can see why it would not make sense to add this to the protocol just for Go debugging. But in that case, would you consider adding something like implementation-specific disconnect attributes that users can specify in json form like they do with launch/attach settings? We have certain flexibility with free-form implementation-specific attributes via launch.json when starting a debug session, but we do not get any such flexibility when ending it.

@puremourning
Copy link
Contributor

implementation-specific disconnect attributes that users can specify in json form like

please can we try to avoid this?... my users are already baffled by the bewildering number of configurations they have to maintain and the sporadically documented debug adapters, particularly when they are not using VSCode (for which most documentation is written; little is written independently of the vscode plugins which manipulate the user config, leading to independent clients having to document server complexities on their behalf and to support users in configuring launch settings). Adding another unspecified section that users are expected to configure and understand will increase this burden (on users and client authors) considerably.

I think your first paragraph sums it up quite well. DAP is an abstraction allowing for a general workflow for any language. The use case for delve mentioned doesn’t seem to be that general. I’m not even sure that long-running/multiple session gdbserver/lldb-server/debugpy persist breakpoints across sessions (though I could be wrong about that), but in any case it would be most surprising to attach to a process then inherit breakpoints from someone else’s debug session, no?

i think if the user is manually staring the remote stub server (in this case delve, but could be any of the above too) then it’s not unreasonable that they also manually kill it.

It’s also worth noting that the terminateDebuggee behaviours of various servers differ considerably when it comes to remote attachment. Some do essentially nothing and some just terminate the stub. So I am in favour of some guidance in this area and even a new value for that like ‘terminate the adapter’. But I’m strongly against arbitrary configuration or messages which we expect users to understand.

@polinasok
Copy link
Contributor Author

I’m strongly against arbitrary configuration or messages which we expect users to understand.

There is no reason why the user has to be exposed to the actual underlying json configuration passed between the client and the adapter/debugger. The frontend can choose to ignore it and rely on the defaults (status quo) or channel it to the user in a form of user-friendly prompts, drop-downs, etc.

@weinand
Copy link
Contributor

weinand commented Apr 12, 2021

@polinasok thanks for your detailed input to the discussion.

First one clarification: we do not see DAP as a "debug" protocol but as a "debugger UI" protocol (mostly because the protocol transfers only user visible strings and doesn't specify their semantics).


What you describe above as a typical go/dlv debug flow means from a DAP perspective:

  • a DA server is started manually,
  • debug sessions are started via DAP's "attach" requests,
  • debug sessions are ended via DAP's "disconnect" request, which has the default semantics for "attach" sessions not to terminate the debuggee automatically. What is missing is a way to specify on "disconnect" whether the debuggee should continue execution or should wait for another "attach" debug session.
  • shutting down the DA server can be done manually (because it was started that way), but a nicer way would be to ask users on "disconnect" whether to terminate the DA server.

With the arrival of #175 (and the corresponding UI microsoft/vscode#116731), it is now optionally possible to terminate the debuggee (and the DA server) on disconnect. This partially addresses the last item from above.

Missing is the "suspendDebuggee" option for the "disconnect" request.

@polinasok is this a correct assessment?

@weinand weinand modified the milestones: March 2021, April 2021 Apr 18, 2021
@weinand
Copy link
Contributor

weinand commented Apr 18, 2021

I think it makes sense to have an option of the disconnect request to control whether the debuggee continues execution after the debugger disconnects, or whether the debuggee stays suspended until another debugger attaches.

I propose to add a new boolean suspendDebuggee property to the disconnect request and a corresponding DA capability supportsSuspendDebuggee. If the capability is true, a client can offer a "Disconnect Suspended" command.

If there are no objections, I would add this DAP feature for the April release.

@polinasok
Copy link
Contributor Author

Sorry for the delayed response. I was taking time off and did not have time to finish my responses to your previous questions.

What you describe above as a typical go/dlv debug flow means from a DAP perspective:

Dlv now has two versions, so the DAP perspective results in somewhat different flows.

  • dlv-rpc: original dlv debugger that requires a separate DA to translate DAP<=>RPC; it takes a program or pid as a command-line argument
  • dlv-dap: new dlv version that understands DAP and is therefore a DA itself; it waits for launch or attach request to start debugging a process
  • a DA server is started manually
  • debug sessions are started via DAP's "attach" requests,

Yes and no

  • dlv-rpc: the DA issues an "attach" request with special mode with "host"/"port" attributes to connect to an already running debugger at host:port, which is already debugging a process, so no program or pid is specified via the DA
  • dlv-dap: "host"/"port" can be used to connect to a manual DA server from the extension, BUT then we still need BOTH "launch"+program OR "attach"+pid requests to tell this external DA server to either launch a new process or attach to an existing one. So while we are attaching to a DA server, we are not necessarily attaching to a process, but instead could be launching one. Does that still constitute an attach request in a traditional sense? Any subsequent client could then do something more akin to what happens with dlv-rpc, where the client attaches to a DA server and inherits an existing debuggee.
  • debug sessions are ended via DAP's "disconnect" request, which has the default semantics for "attach" sessions not to terminate the debuggee automatically.

When we are dealing with languages where the running program is separate from its debugger, attach can have multiple meanings:
A. attach to a debugger (launched manually) that already launched a process
B. attach to a debugger (launched manually) and ask it to launch a process
C. attach to a debugger (launched manually) that already attached to a process
D. attach to a debugger (launched manually) and ask it to attach to a process
E. launch a debugger (from the extension or separate built-in or external DA) and ask it to attach to a process

Since for all of the above, the disconnect semantics would mean different things (at least by default), we essentially end up deciding behind the scenes based on a combination of attach attributes, but also other internal settings (e.g. specified on the command-line when the debugger was started). For example,
A/B + server accepts only one client => terminate debugger, terminate process
A/B + server accepts multiple clients to debug the same process => keep both
C/D + server accepts only one client => terminate debugger, keep process
C/D + server accepts multiple clients to debug the same process => keep both
E + server accepts one client => terminate debugger, keep process
E + server accepts multiple clients to debug the same process => keep both? reject such a configuration in the first place because here the extension controls the lifecycle and host/port are internal?

The spec says "asks the debug adapter to disconnect from the debuggee and to terminate the debug adapter".
So it seems that it really could mean any of the following:

  1. terminate the target process and any runtime or debugger connected to it + terminate the DA
  2. let the target process continue, but shut down the debugger + terminate the DA
  3. let the target process and the debugger stay alive and continuing execution + terminate the DA
  4. let the target process and the debugger stay alive but halt the execution + terminate the DA
    Those are the options from the dlv user point of view. With the addition of the new suspendDebuggee option, it looks like additional options are implied where the target pauses or continues in the absense of an attached debugger.

So from the above, the spec seems to overload the meaning of debuggee, where it can refer to target process or to target+debugger pair.

What is missing is a way to specify on "disconnect" whether the debuggee should continue execution or should wait for another "attach" debug session.
Missing is the "suspendDebuggee" option for the "disconnect" request.

Yes, but is it the only one though? How does one differentiate between letting the process run with the debugger attached vs letting the process run with the debugger attached? Hence issue #175.

  • shutting down the DA server can be done manually (because it was started that way), but a nicer way would be to ask users on "disconnect" whether to terminate the DA server.

Yes, this is a nice to have. The server could be killed manually.

With the arrival of #175

Is #175 (terminateDebugger, not terminateDebuggee) arriving? microsoft/vscode#116731 gives us a binary Stop/Disconnect choice, which is translated to adding terminateDebuggee to the disconnect request, a great feature, but not a replacement for #175, isn't it?

it is now optionally possible to terminate the debuggee (and the DA server) on disconnect

Should we think of the debuggee as the target process? Or as the process+debugger?

As a user, I think of debuggee as the running program being debugged. But as I analyzed above, it appears that in DAP the debuggee can in some cases be bundled with the debugger from the point of view of the spec and the client.

On the other hand the spec also says "terminateDebuggee indicates whether the debuggee should be terminated when the debugger is disconnected", which suggests that debuggee is just the target, not target + debugger.

@polinasok
Copy link
Contributor Author

Thank you for working on this and for the discussion!

I think it makes sense to have an option of the disconnect request to control whether the debuggee continues execution after the debugger disconnects, or whether the debuggee stays suspended until another debugger attaches.

In the Go world, we suspend the debuggee by not disconnecting the debugger, but instead by leaving it connected and in halted state and available to accept more client connections. I guess one could claim that this is a hidden implementation detail, and the user doesn't need to know if a new debug session means reusing the same debugger or attaching a new one. And in that case I think the spec should say "continue or suspend execution after the DA disconnects", not "the debugger disconnects", shouldn't it? Or is the debugger disconnect a requirement here? What if the user doesn't want it to disconnect and does want it up and running? How would the implementors of DAP give a user two option to not suspend the process (aka let execution continue) with or without a debugger attach?

@weinand
Copy link
Contributor

weinand commented Apr 20, 2021

@polinasok yes, the wording "debugger disconnects" in this paragraph:

I think it makes sense to have an option of the disconnect request to control whether the debuggee continues execution after the debugger disconnects, or whether the debuggee stays suspended until another debugger attaches.

was confusing. What I wanted to say was:

I think it makes sense to have an option of the disconnect request to control whether the debuggee continues execution after the client debugger UI disconnects, or whether the debuggee stays suspended until another debugger attaches.

With DAP we are not talking about the "debugger", because that's an implementation detail of the DA.

And you are correct that #175 is not arriving for reasons explained here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests

3 participants