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

dlv dap: --accept-multiclient and --continue support #2323

Closed
polinasok opened this issue Jan 24, 2021 · 10 comments
Closed

dlv dap: --accept-multiclient and --continue support #2323

polinasok opened this issue Jan 24, 2021 · 10 comments

Comments

@polinasok
Copy link
Collaborator

polinasok commented Jan 24, 2021

With headless dlv, the --accept-multiclient feature allows for multiple clients to connect to a server debugging a single process. It is used in 3 scenarios:

  1. To launch/attach to a remote process and use --continue (triggered by a quick behind-the-scenes client)
  2. To connect, disconnect and connect again to a debugging server, usually in a remote container, for repeated debugging sessions or accidental dropped connections
  3. To connect multiple clients concurrently to debug the same process.
    This comes into play when a user hits a limitation in vscode UI. In that case, they might also connect with dlv-connect or even GoLand to take advantage of their additional features (e.g. to use config command, print variables in flattened text form, etc). We recommend this method when users ask to expose dlv cli features via the Debug Console (which is non-trivial due to the split architecture). And we have been considering streamlining the double client option via the extension to help with this use case (as opposed to reimplementing the dlv cli on the adapter/debugger side). This mode comes with limitations and risks due to lack of client UI synchronization - see accept-multiclient: concurrent clients can get the server and each other stuck #2322.

In all cases, delve starts debugging a process as part of server start-up outside of any client. And with this option, the server doesn't automatically exit when any and all client connections are gone, but can continue running, with the target suspended or running, waiting for another client to connect.

Unfortunately, this model was not considered by DAP designers (as recently confirmed with @weinand offline). First of all, DAP clients are supposed to always send a launch/attach request to start the debug session, which doesn't make sense if the debugger already launched/attached to a process. Secondly, there is no DAP request to add a breakpoint to the list of already set breakpoints. There is only a setBreakpoint request, which clears all previous breakpoints and sets all specified breakpoints. That means that concurrent and sequential clients will clear each other's breakpoints. Thirdly, it will be tricky if not impossible to keep the clients' UIs in sync with what is actually happening, which might lead to untimely requests and issues like in #2322. We might be able to accomplish some UI synchronization with optional DAP events like continued event, but with DAP not actively supporting this use case, there will likely be situations where the right events are not available.

We could consider separating support for sequential (covers 1 & 2 above) and parallel (3 above) clients and only support the sequential case to aid remote debugging to begin with. I would even argue that the two cases belong behind two different flags, but since the flag is already in use and comes with certain expectations, we could reject concurrent clients by sending an explicit error message in response to their initialize requests to let the users know that this use case is not (yet?) supported.

@aarzilli
Copy link
Member

For dlv dap anything is fine, for other commands of dlv the function of --accept-multiclient has to stay the same, for backwards compatibility. I think at some point the ability to connect with both DAP and a command line client simultaneously was proposed. I don't remember where/when/who but if that's implemented it would also have all of this problems.

@polinasok
Copy link
Collaborator Author

polinasok commented Jan 27, 2021

I understand. I am not even sure it is a good idea to define very different flags for DAP because --accept-multiclient is a global flag and is inherited by dap mode by default. I am debating between the following options:

  • (A) For now implement --accept-multiclient to only allow sequential clients debugging the same process. Print a warning on start-up that parallel clients are not yet supported. Return an error if clients connect in parallel. Eventually hopefully support both with the same flag like traditional dlv.
  • (B) Not use --accept-multiclient and define --accept-sequential-clients and --accept-concurrent-clients.

Again both are for the case where the debugger is tracking a single process. There is also a proposal for the debugger to be a long-running server that is to stay running on disconnect to accept more client connections, initiating a new debug session to launch/attach to a new process (#2329).

@polinasok
Copy link
Collaborator Author

@hyangah has expressed a concern that under use case (3) above, the ability to connect multiple client simultaneously won't be very useful because these alternative clients do not speak DAP. See also discussion in #2328.

@polinasok
Copy link
Collaborator Author

polinasok commented Nov 1, 2021

Instead of adding separate --accept-multiclient and --continue support to dlv dap, we have decided to support both protocols on the same channel with the original dlv commands that already support those flags. The dap server has been upgraded to accept remote attach connections to an already running debugger session and correctly shut things down in multi-client mode. The original dlv commands (that launch/attach the debuggee before starting up the server) now start up a common server that detects the request protocol on the fly. In multi-client mode, a quick rpc client is still used to optionally continue on start-up, but otherwise the server can accept both rpc and dap clients in a sequence or at the same time (with the same risks and lack of client UI synchronization as before - see #2322).

@polinasok polinasok changed the title dlv dap: --accept-multiclient support dlv dap: --accept-multiclient and --continue support Nov 1, 2021
@aarzilli
Copy link
Member

aarzilli commented Nov 2, 2021

Do you want to keep this open because of the issues with synchronizing run status and breakpoints between multiple connected clients?

@polinasok
Copy link
Collaborator Author

I debated closing this and the remote issue yesterday, but decided to do a little bit more testing before I do so. We have not been planning to synchronize the UI, at least not in the short term. What do you think?

@aarzilli
Copy link
Member

aarzilli commented Nov 2, 2021

I think it's fine to not do it.

@gustavomassa
Copy link

Is there a way to use the --continue with the dap? I want to use dap but I don't want to wait for a client to connect before continuing the code. Using the dlv exec with --accept-multiclient and --continue works perfectly.

@polinasok
Copy link
Collaborator Author

Yes, there is a way. And we are anxiously waiting for user feedback to iron out any kinks there might be.
Starting with dlv 1.7.3 dlv exec --headless --accept-multiclient --continue now starts a server that can talk to both RPC and DAP clients. All you need to do to use DAP features is connect with a DAP client. For example, please see documentation on the vscode-go side.

gopherbot pushed a commit to golang/vscode-go that referenced this issue Dec 29, 2021
Updates go-delve/delve#2323

Change-Id: I4c89c299ffa569bb3ecbcb0e730f2f7ad04e16d8
GitHub-Last-Rev: 26bb7f9
GitHub-Pull-Request: #1979
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/374615
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Polina Sokolova <polina@google.com>
@polinasok
Copy link
Collaborator Author

I am going to close this issue. The feature got released in both dlv and vscode-go, so if users run into problems, they can file new issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants