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

debug: supports disconnect/restart while program is running #1347

Closed
hyangah opened this issue Mar 15, 2021 · 6 comments
Closed

debug: supports disconnect/restart while program is running #1347

hyangah opened this issue Mar 15, 2021 · 6 comments
Assignees
Labels
Debug Issues related to the debugging functionality of the extension. FrozenDueToAge

Comments

@hyangah
Copy link
Contributor

hyangah commented Mar 15, 2021

How to reproduce:

simple program

package main
import "time"
func main() {
   time.Sleep(1*time.Hour)
}

Launch debugging (F5) with "debugAdapter": "dlv-dap" in launch.json

While the program is sleeping, restart debugging (⇧⌘F5 on mac). See the previous debug session is terminated but the new debug session doesn't start. (Sometimes I get connection refused error popup but not always)

@hyangah hyangah added Debug Issues related to the debugging functionality of the extension. DlvDAP labels Mar 15, 2021
@hyangah
Copy link
Contributor Author

hyangah commented Mar 22, 2021

This happens only when the debugged program is "RUNNING". If we set a breakpoint, the restart triggers a 'disconnect' command first to the debug adapter before restarting.
The main issue is that dlv dap is not yet able to accept additional requests while the program is RUNNING and the handler is blocked. I guess @polinasok is already working on the related issue.

Another issue is that, after sending the disconnect request, VSCode calls debug adapter descriptor factory's createDebugAdapterDescriptor again, with the exact same DebugSession object. This call happens before it receives the disconnect response. I don't know if that's a bug or an intended behavior yet. But the DebugSession object passed here was modified during the previous call (i.e. we set 'port' attribute), and as a result, VSCode tries to contact the same dlv dap server using the port. As a result - if dlv dap is dead somehow, restart will fail with the connection refused error. If dlv dap is alive still stuck working for the previous run, VSCode will stuck in the DebugAdapterServer waiting for the dlv dap to answer.

The first step is to fix the delve to accept more requests.

cc/ @suzmue @polinasok

@polinasok
Copy link
Contributor

That's right: disconnecting while running is blocked on async support that is work in progress.

It is odd that vscode doesn't wait for disconnect response before calling the factory code again to relaunch. According to the DAP spec, a client should "implement 'restart' by terminating and relaunching the adapter", which to me implies actually waiting for the indication that the adapter terminated (aka disconnect response). If that is not the case, I would file a bug against vscode.

In any case, when the factory starts up a new server, I think it should always use a new port. Even if our code times everything right, we are still at the mercy of the OS. The port might take a moment to free up. This actually reminds me of microsoft/debug-adapter-protocol#126.

Does vscode set restart in DisconnectArguments?

@hyangah hyangah changed the title debug: restart debug in dlv-dap mode isn't working debug: supports disconnect/restart while program is running Mar 25, 2021
@hyangah
Copy link
Contributor Author

hyangah commented Mar 29, 2021

Thanks @polinasok

It is odd that vscode doesn't wait for disconnect response before calling the factory code again to relaunch.

Unsurprisingly, there is a timeout. Since dlv dap didn't respond timely, VS Code went ahead, continued remaining cleanup tasks (socket close, adapter dispose, ...), and restarted.

According to the DAP spec, a client should "implement 'restart' by terminating and relaunching the adapter", which to me implies actually waiting for the indication that the adapter terminated (aka disconnect response). If that is not the case, I would file a bug against vscode.

I think the timeout from the client side is a reasonable defense mechanism in this case.

In any case, when the factory starts up a new server, I think it should always use a new port.

Yes, I will prepare a fix.

Does vscode set restart in DisconnectArguments?

Yes.

@gopherbot
Copy link
Collaborator

Change https://golang.org/cl/305552 mentions this issue: src/goDebugFactory: don't modify DebugConfiguration.port

gopherbot pushed a commit that referenced this issue Mar 29, 2021
This is a change to avoid modifying vscode.DebugConfiguration's
port attribute inside startDapServer function.
VSCode invokes DebugAdapterDescriptorFactory's
createDebugAdapterDescriptor with the same DebugSession
& DebugConfiguration objects when the session restart is
requested. If we mess with the port attribute, the next call
will be handled as if it was requested to connect to an external
DAP server. So, let's stop modifying it.

Updates #1347

Change-Id: I82d7b7f48924fd36e7a881be39fedf2e440e4809
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/305552
Trust: Hyang-Ah Hana Kim <hyangah@gmail.com>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
@suzmue suzmue added this to the v0.24.0 milestone Mar 31, 2021
@hyangah hyangah modified the milestones: v0.24.0, Backlog, On Deck Apr 6, 2021
@hyangah hyangah modified the milestones: On Deck, Untriaged, Backlog Apr 14, 2021
@polinasok
Copy link
Contributor

Both disconnect and restart for me now.

@hyangah
Copy link
Contributor Author

hyangah commented May 25, 2021

Thanks @polinasok.

@hyangah hyangah closed this as completed May 25, 2021
@golang golang locked and limited conversation to collaborators May 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Debug Issues related to the debugging functionality of the extension. FrozenDueToAge
Projects
None yet
Development

No branches or pull requests

4 participants