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: revisit remote attach disconnect/shutdown implementation #748

Closed
polinasok opened this issue Oct 6, 2020 · 2 comments
Closed

debug: revisit remote attach disconnect/shutdown implementation #748

polinasok opened this issue Oct 6, 2020 · 2 comments
Assignees
Labels
Debug Issues related to the debugging functionality of the extension. FrozenDueToAge

Comments

@polinasok
Copy link
Contributor

polinasok commented Oct 6, 2020

We must differentiate between single- and multi-client debug server when stopping the debug session and disconnecting. Our code appears to be written with the assumption that the remote server is always run with --accept-multiclient, so instead of halting and detaching, it just closes the rpc connection to the server (see here). Leaving the server running makes sense only if it can accept more client connections. Otherwise, we will have a lingering server that is unresponsive and can no longer be connected to (see #497). If the server is not multi-client (check with RPCServer.IsMulticlient), it should be killed with halt+detach on disconnect. When the server is killed, we should follow the same pattern as local launch/attach, killing the target process if it was launched with dlv debug, and letting it run if it was attached to with dlv attach.

We should fix the logging in the code to accurately log when we halt+detach and kill the server/process and when we just close the connection and leave them running. Currently we print "HaltRequest" even when we do not halt, which makes the logs quite confusing.

Optionally we could extend the above default behavior with an additional option to kill the server. This is what
dlv cli does interactively. I do not know of an interactive way to do this in vscode, but we could come up with a launch.json attribute to specify upfront that the server should be terminated on disconnect. Also filed microsoft/debug-adapter-protocol#175 to get this flag into DAP and then vscode.

(dlv) c
Ctrl-c
Would you like to [p]ause the target (returning to Delve's prompt) or [q]uit this client (leaving the target running) [p/q]? 
(dlv) p
(dlv) exit
Would you like to kill the headless instance? [Y/n] y
Would you like to kill the process? [Y/n]

Interestingly, vscode-go adapter always restarts a halted target process on disconnect while dlv cli does not. This could be another option to add to the debug configuration. Also filed microsoft/debug-adapter-protocol#177 to get this flag into DAP and then vscode.

The shutdown behavior can be explored with the following simple program

func main() {
	for i := 0; true; i++ {
		fmt.Println("================== i", i)
		time.Sleep(2 * time.Second)
	}
}

And with the following launch.json configuration comparing to dlv connect :2345:

            "name": "Connect to server",
            "type": "go",
            "request": "attach",
            "mode": "remote",
            "remotePath": "${workspaceFolder}",
            "port": 2345,
            //"trace":"verbose",
            "host": "127.0.0.1"

Then the cases are as follows:

dlv debug foo.go --headless —api-version=2 --listen=:2345 --accept-multiclient [--continue]
  • TODO: Clearly log that we are only closing connection (so users know they need to clean up the server)
  • TODO: Optionally add a flag to terminate server (which also terminates the target)
./foo.go; dlv attach <foopid> --headless —api-version=2 --listen=:2345 --accept-multiclient [--continue]
  • TODO: Clearly log that we are only closing connection (so users know they need to clean up the server)
  • TODO: Optionally add a flag to terminate server (which does not terminate the target by default)
  • TODO: See if supportTerminateDebuggee would give us a prompt to terminate the target
dlv debug foo.go --headless —api-version=2 --listen=:2345
  • TODO: Use halt+detach to terminate server (which also terminates the target)
./foo.go; dlv attach <foopid> --headless —api-version=2 --listen=:2345
  • TODO: Use halt+detach to terminate server (which does not terminate the target)
  • TODO: See if supportTerminateDebuggee would give us a prompt to terminate the target

@quoctruong

@polinasok polinasok added Debug Issues related to the debugging functionality of the extension. DelveDAP labels Oct 6, 2020
@polinasok polinasok changed the title debug: Revisit remote attach disconnect/shutdown implementation debug: revisit remote attach disconnect/shutdown implementation Oct 9, 2020
@polinasok
Copy link
Contributor Author

Interesting issue filed against dlv suggesting that an interactive prompt on exit is too annoying: go-delve/delve#2324.

@polinasok
Copy link
Contributor Author

We do not plan to fix this in the legacy adapter, which is being replaced by dlv-dap. With the master version of dlv, which now understands DAP requests and support remote attach, we have:

Multi-Client Launch or Attach Debugger

./dlv debug foo.go --headless --accept-multiclient [--continue]
./foo.go; ./dlv attach <foopid> --headless --accept-multiclient [--continue]
Client Debugger Target Use
dlv debug/exec Stop Keep Keep Disconnect (Default) image
dlv debug/exec Stop Stop Keep not possible
dlv debug/exec Stop Stop Stop Stop image
dlv attach Stop Keep Keep Disconnect (Default) image
dlv attach Stop Stop Keep Ctrl-C or kill from terminal to stop dlv
dlv attach Stop Stop Stop Stop image

Single-Client Launch or Attach Debugger

./dlv debug foo.go --headless
./foo.go; ./dlv attach <foopid> --headless
Client Debugger Target Use
dlv debug/exec Stop Keep Keep not possible
dlv debug/exec Stop Stop Keep not possible
dlv debug/exec Stop Stop Stop Disconnect (Default) or Stop image
dlv attach Stop Keep Keep not possible
dlv attach Stop Stop Keep Disconnect (Default) image
dlv attach Stop Stop Stop Stop image

@golang golang locked and limited conversation to collaborators Oct 28, 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

3 participants