cmd/dlv: add --client-addr flag to run dap with a predefined client

[hyangah]

This PR adds a new --client-addr=host:port flag to dlv dap.
If it is supplied, the dap process will dial into the tcp port where
a DAP client is waiting, and work with only the DAP client.
The DAP client is supposed to start the normal DAP message
exchange starting with the 'initialize' request after the dlv dap
process dials in and the connection is set up.

VS Code Go extension plans to use this mode for

• reliably detecting dlv dap readiness. Currently it depends on
  watching the log stream. After this PR, it can listen on a network port.
• running dlv dap from any terminal (part of RunInTerminal workflow
  implementation).

Update golang/vscode-go#124

[aarzilli]

LGTM

[hyangah]

@polinasok @suzmue

[suzmue]

LGTM

[polinasok]

Pardon the delay. It took me a couple of tries to understand what's going on and remind myself what RunInTermnial was about. Have I, by any chance, missed a design discussion for this design in some doc or issue?

Do I understand correctly that you are trying to achieve the following?

+--------+           +---------------+
│ vscode │           │ dlv-dap @addr │
+--------+           +---------------+
            launch req       
            =======>  
       runInTerminal req 
    (dlv dap --connect=addr)
            <======

starts . . . . . . . . . . . . . . . . . . . . . in  terminal
                                                +------------------------+
                                                │$ dlv dap --connect=addr│
                                                +------------------------+
        runInTerminal resp 
        (dlv pid, shell pid)
            =======>  


                      ignores pids b/c has tcp conn
                                      initialize req
                                      =======>
                                      initialize res
                                      <=======
                                      launch req
                                      =======>
                                                  launches debugger+program
         initialized event            initialized event
             <=======  . . . . . . .  <=======
          launch res                  launch res
             <=======  . . . . . . .  <=======
                     forwards everything else  
                                                  does the actual debugging     

Some high-level questions come to mind:

1. Spec says This is typically used to launch the debuggee in a terminal provided by the client. So from that and the pid in the args I get an impression that we should be considering starting just the target process in he terminal and then having the original dlv-dap attach to it. Is this something you have considered? I guess one thing that complicates this is that we will need to start the process suspended (no idea how/if it's possible), so it doesn't do anything useful before he debugger attaches. Not sure if there might also be any permissions issues.
2. Have you found any good examples of what other adapters are doing (launching debugger or debuggee, etc)?
3. Is it possible to hook-up a terminal using the new --tty flag?
4. Will we need this to work with --accept-multiclient?

[polinasok]

This reminded me of the preconnected Listener used by the rpc server. In non-headless (basically with-client) mode, even though the client connection is predefined, the server code is reused as-is. Is something similar perhaps to consider here to arrive at a less nuanced common server denominator?

[hyangah]

Sorry, I don't understand what you mean. There is nothing special here beyond the standard basic tcp connection and I don't see much of commonality beyond that.

preconnected Listener is used to arrange the dlv cli to connect to the server (possibly running in process). There, the client is still a client (the role in the rpc protocol) and the server is what uses service/debugger package. In this reverse mode, the entity that is dialing and plays the role of 'server' in the protocol, is what's using service/debugger package.

[polinasok]

In my understanding, the client is the entity that makes the debugging requests and the server is the entity that serves those requests. I don't think it matters which side connects to which side, client to server or server to client. Regardless of the specifics of that initial handshake, the rest of the traffic and meaning of the two sides don't change, do they? That's true for both rpc+terminal case and for dap+special-client case.

In this new mode you are defining, you have a predefined client that you connect to before you call NewServer.
In case of the rpc server + terminal client, we also have a client connection that is predefined and pre-connected before newServer is called. Yet newServer still takes a listener, but that listener is fake. The listener doesn't actually listen as it is already pre-connected to the connection point that was already set up. On the very first call to Accept it will return that pre-existing client connection. And because of that abstraction, newServer and Run can be re-used as-is without any special logic or comments inside of the server itself.

I propose that we do the same here to move connection details outside of the server, keeping it general and consistent with the rpc server, which we ultimately want to merge with under a single command.

listenerpipe.go has ListenerPipe + generic preconnectedListener, which you can reuse more or less ass-is. You can define something like this:

func ListenerFromTCPClient(clientAddr string) net.Listener { 
         conn, err := net.Dial("tcp", clientAddr)
	return &preconnectedListener{conn: conn, closech: make(chan struct{})}
}

Then pass the result to NewServer and not need to modify the dap server code.

[hyangah]

I think the proposed approach overcomplicates the code without clear use cases. When the use case that benefits from it is obvious, I am ok to rework this. Until then, I prefer simplicity and less channels and locks.

[polinasok]

In my understanding, the client is the entity that makes the debugging requests and the server is the entity that serves those requests. I don't think it matters which side connects to which side, client to server or server to client. Regardless of the specifics of that initial handshake, the rest of the traffic and meaning of the two sides don't change, do they? That's true for both rpc+terminal case and for dap+special-client case.

In this new mode you are defining, you have a predefined client that you connect to before you call NewServer.
In case of the rpc server + terminal client, we also have a client connection that is predefined and pre-connected before newServer is called. Yet newServer still takes a listener, but that listener is fake. The listener doesn't actually listen as it is already pre-connected to the connection point that was already set up. On the very first call to Accept it will return that pre-existing client connection. And because of that abstraction, newServer and Run can be re-used as-is without any special logic or comments inside of the server itself.

I propose that we do the same here to move connection details outside of the server, keeping it general and consistent with the rpc server, which we ultimately want to merge with under a single command.

listenerpipe.go has ListenerPipe + generic preconnectedListener, which you can reuse more or less ass-is. You can define something like this:

func ListenerFromTCPClient(clientAddr string) net.Listener { 
         conn, err := net.Dial("tcp", clientAddr)
	return &preconnectedListener{conn: conn, closech: make(chan struct{})}
}

Then pass the result to NewServer and not need to modify the dap server code.

[hyangah]

Some high-level questions come to mind:

1. Spec says This is typically used to launch the debuggee in a terminal provided by the client. So from that and the pid in the args I get an impression that we should be considering starting just the target process in he terminal and then having the original dlv-dap attach to it. Is this something you have considered? I guess one thing that complicates this is that we will need to start the process suspended (no idea how/if it's possible), so it doesn't do anything useful before he debugger attaches. Not sure if there might also be any permissions issues.

Your guess is correct. I don't see a way to do that correctly in a simple way. Permission issue (since one of the goal is to debug as a root) is also a bummer.

2. Have you found any good examples of what other adapters are doing (launching debugger or debuggee, etc)?

I hope golang/vscode-go#1626 answered this question.

3. Is it possible to hook-up a terminal using the new --tty flag?

Yes. golang/vscode-go#1626 also discussed this option. LLDB adapter uses a similar approach.

I don't find the option very attractive given all the diversity in users' platforms and the amount of work necessary to implement the terminal agent for unix/windows. That doesn't help the solution for debugging-as-a-root so we will need to figure out another way to solve the issue. And recently I want vscode-go extension to use this mode by default and avoid the dependency on the log output to determine the readiness of dlv dap server.

4. Will we need this to work with --accept-multiclient?

No.

[polinasok]

Thank you for simplifying.

[hyangah]

PTAL

Added RunWithClient separate from Run - hoping that ultimately this can be implemented directly using the new Session and bypassing the Server layer.

[polinasok]

This could be moved to Run(). Technically you are not listening until then anyway. Maybe it would even be cleaner to have Run() take listener as arg. I am not yet full convinced we should strip the server layer in this case and if we keep it.

[hyangah]

I agree.

The only issue is the log from WriteDAPListeningMessage must be the first log output. So, it's better to keep it close to where the logger is created. We can move logger creation to Run too. What do you think?

[polinasok]

Why not handle this misconfiguration the same as in Run?

[hyangah]

I think this is misuse of API, so I want to handle it as a hard failure - not just error out and shutdown like normal server shutdown. The misconfig in Run is also misuse of API - if you are fine to make it fatal, I am happy to make the change.

[hyangah]

Anything else to address?

[derekparker]

LGTM - please just rebase and then we can merge after @polinasok +1.

[derekparker]

Re-running CI as a sanity check.