Attached debugger does not remove breakpoints before disconnecting

[OrBin]

What version of Go, VS Code & VS Code Go extension are you using?

Version Information

Go 1.18
VS Code 1.69.2
VS Code Go 0.35.1

Share the Go related settings you have added/edited

Default settings

Describe the bug

Using a remote debugging configuration with "attach" request (from launch.json), attaching the debugger, defining a breakpoint, then clicking "disconnect", the debugee continue running (as expected), but still has the breakpoints set, so that it stops in the breakpoints. This is a weird, unexpected behavior. GoLand, for example, sends ClearBreakpoint commands to dlv server for all registered breakpoints.
Instead, vscode-go just closes the connection without letting the process go back to the previous (=breakpointless) state.
For now, I worked around this with a script I added as a postDebugTask to open a socket to dlv server and clear all breakpoints. This is not a sustainable solution though.

Here is the relevant section in the code, AFAICT:

vscode-go/src/debugAdapter/goDebug.ts

Lines 830 to 839 in 944b1d4

	// For remote debugging, we want to leave the remote dlv server running,
	// so instead of killing it via halt+detach, we just close the network connection.
	// See https://www.github.com/go-delve/delve/issues/1587
	if (this.isRemoteDebugging) {
	log('Remote Debugging: close dlv connection.');
	const rpcConnection = await this.connection;
	// tslint:disable-next-line no-any
	(rpcConnection as any)['conn']['end']();
	return resolve();
	}

Steps to reproduce the behavior:

1. Add this configuration to launch.json:

{
    "name": "MyApp (attach)",
    "type": "go",
    "request": "attach",
    "debugAdapter": "dlv-dap",
    "mode": "remote",
    "substitutePath": [
        { "from": "${workspaceFolder}", "to": "/code" },
        { "from": "${env:GOROOT}", "to": "/usr/local/go"},
        { "from": "${env:GOPATH}", "to": "/root/go"},
    ],
    "host": "debugee_host",
    "port": <debugee_port>,
    "showLog": true,
    "trace": "log",
    "logOutput": "debugger,gdbwire,lldbout,debuglineerr,rpc,dap,fncall,minidump"
}

2. Choose this configuration
3. Click "Start Debugging (F5)"
4. Add a breakpoint
5. Make the app run the code with the breakpoint and see VS Code stopping in the breakpoint
6. Continue
7. Click "Disconnect"
8. Make the app run the code with the breakpoint again and see that it stops even though VS Code is not even connected to dlv server.

[hyangah]

Yes, dlv dap is not clearing breakpoints when disconnecting.

go-delve/delve#2772 and go-delve/delve#2958 (comment) discuss this issue but I don't know what's the current status of Delve DAP.

@polinasok Is there any reason that the dlv dap shouldn't follow the behavior of the legacy debug adapter?

This is a regression compared to the legacy debug adapter, and this needs to be resolved before we completely switch over to the dlv-dap adapter. @briandealwis @suzmue

[OrBin]

Yes, dlv dap is not clearing breakpoints when disconnecting.

go-delve/delve#2772 and go-delve/delve#2958 (comment) discuss this issue but I don't know what's the current status of Delve DAP.

@polinasok Is there any reason that the dlv dap shouldn't follow the behavior of the legacy debug adapter?

Do you mean that the issue is dlv dap itself leaving the debugee with the breakpoints, rather than vscode-go?
What do you think about adding a workaround in vscode-go, cleaning all breakpoints explicitly before closing the connection? (at least until this is resolved in dlv)

This is a regression compared to the legacy debug adapter, and this needs to be resolved before we completely switch over to the dlv-dap adapter. @briandealwis @suzmue

I think it's already too late for this; Some bugs have been fixed for DAP only, so the legacy adapter is also not fully usable in that case, which leaves no choice for some users to switch over to the dlv-dap adapter.

[hyangah]

Yes, dlv dap is not clearing breakpoints when disconnecting.
go-delve/delve#2772 and go-delve/delve#2958 (comment) discuss this issue but I don't know what's the current status of Delve DAP.
@polinasok Is there any reason that the dlv dap shouldn't follow the behavior of the legacy debug adapter?

Do you mean that the issue is dlv dap itself leaving the debugee with the breakpoints, rather than vscode-go? What do you think about adding a workaround in vscode-go, cleaning all breakpoints explicitly before closing the connection? (at least until this is resolved in dlv)

@OrBin dlv dap is the debug adapter our team developed in collaboration with the delve team. This should be fixed inside dlv dap. Will discuss this in our team how we can prioritize this.

[OrBin]

Thanks @hyangah. Do you have any expectation regarding whether this is going to be done and when?

[polinasok]

This is a regression compared to the legacy debug adapter, and this needs to be resolved before we completely switch over to the dlv-dap adapter.

@hyangah Where are you seeing this legacy behavior? We generally tried to follow its example where it made sense. I do not see any clearing by the legacy adapter in the code

vscode-go/src/debugAdapter/goDebug.ts

Lines 1008 to 1015 in 84fe4e7

	// For remote process, we have to issue a continue request
	// before disconnecting.
	if (this.delve?.isRemoteDebugging) {
	if (!(await this.isDebuggeeRunning())) {
	log("Issuing a continue command before closing Delve's connection as the debuggee is not running.");
	this.continue();
	}
	}

vscode-go/src/debugAdapter/goDebug.ts

Lines 830 to 838 in 84fe4e7

	// For remote debugging, we want to leave the remote dlv server running,
	// so instead of killing it via halt+detach, we just close the network connection.
	// See https://www.github.com/go-delve/delve/issues/1587
	if (this.isRemoteDebugging) {
	log('Remote Debugging: close dlv connection.');
	const rpcConnection = await this.connection;
	// tslint:disable-next-line no-any
	(rpcConnection as any)['conn']['end']();
	return resolve();

Here is a log when I click Continue, then Disconnect while printing numbers in a loop with conditional breakpoint in the middle of the range.

2022-08-02T14:18:04-07:00 debug layer=rpc (async 45) <- RPCServer.Command(api.DebuggerCommand{"name":"continue","ReturnInfoLoadConfig":null})
2022-08-02T14:18:04-07:00 debug layer=debugger continuing
2022-08-02T14:18:05-07:00 debug layer=rpc (async 46) <- RPCServer.State(rpc2.StateIn{"NonBlocking":true})
2022-08-02T14:18:05-07:00 debug layer=rpc (async 46) -> rpc2.StateOut{"State":{"Pid":0,"Running":true,"Recording":false,"CoreDumping":false,"Threads":null,"NextInProgress":false,"WatchOutOfScope":null,"exited":false,"exitStatus":0,"When":""}} error: ""
### Client already disconnect, the numbers are printing one by one, then stop at a conditional breakpoint (i==50)
### Dlv server tries to send a response to the stopped continue request, but hits a closed connection - see error below
0 5 10 15 20 25 30 35 40 45 2022-08-02T14:18:16-07:00 debug layer=rpc (async 45) -> rpc2.CommandOut{"State":{"Pid":0,"Running":false,"Recording":false,"CoreDumping":false,"currentThread":{"id":2896125,"pc":17483217,"file":"/Users/polina/go/src/hello/hello.go","line":82,"function":{"name":"main.main","value":17483040,"type":0,"goType":0,"optimized":false},"goroutineID":1,"breakPoint":{"id":2,"name":"","addr":17483217,"addrs":[17483217],"file":"/Users/polina/go/src/hello/hello.go","line":82,"functionName":"main.main","Cond":"i == 50","HitCond":"","continue":false,"traceReturn":false,"goroutine":false,"stacktrace":0,"LoadArgs":{"FollowPointers":true,"MaxVariableRecurse":1,"MaxStringLen":400,"MaxArrayValues":400,"MaxStructFields":-1},"...etc.
2022-08-02T14:18:16-07:00 error layer=rpc writing response:write tcp 127.0.0.1:54321->127.0.0.1:64480: use of closed network connection

[polinasok]

An update here might be of interest: go-delve/delve#2958 (comment).
In short, in the past we discussed that there are users that want either behavior (clear or not), and we should ideally control this by a flag.

I am pretty sure we didn't clear the breakpoints because the legacy adapter didn't, and we generally aimed for feature parity (unless it was a bug or a known pain point). That said, this is a new adapter, and we are in our right to define the default case the way we see fit regardless of what the legacy adapter did or did not do. We can clear and keep async logic simple in v1 and then maybe add an option to keep the breakpoints and update async logic accordingly in v2. Just need to document our decision, so users can let us know if they want the extra option.

@OrBin Another workaround is one-click clearing from the UI (not sure how to trigger automatically though).

[hyangah]

@hyangah Where are you seeing this legacy behavior? We generally tried to follow its example where it made sense. I do not see any clearing by the legacy adapter in the code

Yes, you are right. I misremembered it and this feature was never implemented. There was a prior UX issue report caused by this left-over breakpoints (#497 (comment) including myself). Can we change to clear breakpoints? It is more intuitive and safer. I hope that simplifies delve dap code path too.

Users who want to leave breakpoints around after the debug sessions are over should understand the implication well and explicitly opt in. Using the dlv command line tool is a good way to do so.

[zheng-fan]

Clearing all breakpoints after disconnecting is an intuitive and side-effect-free behavior. Otherwise, the program will get stuck when it reaches a breakpoint, even though we are not debugging.
I sincerely hope it can be addressed in a future update. Thank you for your consideration.