accept-multiclient: concurrent clients can get the server and each other stuck

[polinasok]

When using --accept-multiclient, it is not immediately obvious that concurrent actions of other clients won't be properly reflected at each cli. In particular, when one client enters "continue", its prompt will disappear indicating a running state. But the other client will still have its prompt and can enter more commands that will get buffered and then cause unexpected behavior.

Adding event notifications or special handling in the server to reject client requests when in certain states is too much of an undertaking. I am not even sure how widely used this use case is. So I would propose highlighting this topic in the documentation and maybe printing warnings when a new clients connects that there is already another client connected and that care is needed to coordinate between clients.

Below is an example, where I ran into a situation where things got stuck. I played with this on my Mac, but I think the high-level concerns expressed below are universal, even if the exact specifics of Ctrl-C, etc. behavior vary from system to system.

$ dlv version
Delve Debugger
Version: 1.5.1
Build: $Id: bca418ea7ae2a4dcda985e623625da727d4525b5 $
$ go version
go version go1.14.3 darwin/amd64

1. Start long-running program (something with infinite loop works)
2. Attach server with --headless --accept-multiclient
3. The program is now suspended
4. Connect client A with dlv connect
5. Connect client B with dlv connect
6. Client A enters "c" to continue and waits for it to return

2021-01-22T17:45:52-08:00 debug layer=debugger continuing

7. Client B still shows a prompt as if the session is still suspended
8. Client B enters "c" to continue
9. Server receives 2nd continue and buffers it
10. Both clients are blocked with no prompt
11. Client B enters Ctrl-C to halt:

Would you like to [p]ause the target (returning to Delve's prompt) or [q]uit this client (leaving the target running) [p/q]? p

11. Buffered 2nd continue kicks in on the server side.

2021-01-22T17:48:35-08:00 debug layer=debugger halting
2021-01-22T17:48:35-08:00 debug layer=debugger continuing

12. Client B does not get its prompt back from the 2nd continue.
13. Client A does get its prompt back from the 1st continue.
14. Client A issues "locals" and blocks
15. Both clients are blocked. Ctrl-C in either window does nothing.
16. If I connect another client, I can suspended the execution and let all previous commands return. But even after that all the previous halt attempts kicked in and trying to navigate the prompts, I entered something that made one of the clients quit and the terminal enter some weird mode
    
    There might have ben an accidental "return" mixed in there that causes client cli to reissue the previous command (which is actually quite annoying, but is a separate issue).

[aarzilli]

would it be better to check in debugger.(*Debugger).Command if debugger.isRunning and return an error? Technically it would be backwards incompatible but it will probably be fine. We'll also have to fix #2216.

[polinasok]

I think that's an good idea and I would advocate doing that for blocking commands that are not meant to complete while the program is running or interrupt it if it is. There are other ways to get stuck with these even in single client mode - e.g. golang/vscode-go#762.

[aarzilli]

That can't be done, there are legitimate reasons to make blocking calls while the program is running (i.e. to get notified when it stops running), especially State.

[polinasok]

Do you mean "GetState" by "State"? I have indeed seen it used in this way:

delve/pkg/terminal/terminal.go

Lines 234 to 236 in 25178e2

	// Ensure that the target process is neither running nor recording by
	// making a blocking call.
	_, _ = t.client.GetState()

. This one is special and has both a blocking and non-blocking version anyway.

I was thinking more about requests like those for goroutines, stack, evaluate. We have run into bugs in vscode where those were issued at the wrong time and the client got stuck (e.g. golang/vscode-go#740). The solution was to always check if the state was running first and return an error or a no-op response otherwise. I was thinking that it could be nice to move the safeguard to the server-side. Are such non-command requests ever issued for legitimate reasons while the program is running? The client won't be able to GetState or Halt if they are.

When you proposed adding this check to Command, did you mean disallowing any additional continuing command (next, step, etc) while the process is running? So they don't get buffered and executed unexpectedly?

[aarzilli]

Are such non-command requests ever issued for legitimate reasons while the program is running?

It's possible that a client would do that. This behavior has been around for a long time. Honestly even just doing it for Command is iffy, thinking about it more.

[polinasok]

What if we hide that behind a flag? If set, then return an error when already running, otherwise block. We can use the flag with dlv connect and dap, but let other clients keep the original behavior.