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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 11 commits into from
Oct 13, 2021
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
cbfed32
cmds: add a new dap-reverse command
hyangah Jul 3, 2021
e6f72d2
cmd/dlv: move dap-reverse to a mode of dap command
hyangah Jul 4, 2021
6538d62
cmd/dlv: address comments - made the --connect unhidden
hyangah Jul 6, 2021
d974e4d
address comments and change --connect to --client-addr
hyangah Jul 14, 2021
596c5ba
Merge branch 'master' into dapreverse
hyangah Sep 24, 2021
fa0574a
dap: address polina's comments and simplify
hyangah Sep 24, 2021
4664030
Merge branch 'master' into dapreverse
hyangah Oct 8, 2021
d818a10
fix logging
hyangah Oct 8, 2021
962ccae
Merge branch 'master' into dapreverse
hyangah Oct 11, 2021
104d76b
Merge branch 'master' into dapreverse
hyangah Oct 13, 2021
e58f24b
dap: made program error to fatal error
hyangah Oct 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Documentation/usage/dlv_dap.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ execution is resumed at the start of the debug session.
dlv dap
```

### Options

```
```

### Options inherited from parent commands

```
Expand Down
65 changes: 47 additions & 18 deletions cmd/dlv/cmds/commands.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@ var (
// disableASLR is used to disable ASLR
disableASLR bool

// dapConnect is dap subcommand's hidden flag that sets the rendezvous point address.
// If it is specified, the dap server operates in reverse mode and
// communicate only through the proxy dap server at the rendezvous point.
dapConnect string

// backend selection
backend string

Expand Down Expand Up @@ -188,6 +193,9 @@ While --continue is not supported, stopOnEntry launch/attach attribute can be us
execution is resumed at the start of the debug session.`,
Run: dapCmd,
}
dapCommand.Flags().StringVar(&dapConnect, "connect", "", "host:port of the proxy DAP server. If set, the command starts a DAP server in reverse mode")
dapCommand.Flags().MarkHidden("connect")
hyangah marked this conversation as resolved.
Show resolved Hide resolved

rootCommand.AddCommand(dapCommand)

// 'debug' subcommand.
Expand Down Expand Up @@ -435,26 +443,47 @@ func dapCmd(cmd *cobra.Command, args []string) {
fmt.Fprintf(os.Stderr, "Warning: program flags ignored with dap; specify via launch/attach request instead\n")
}

listener, err := net.Listen("tcp", addr)
if err != nil {
fmt.Printf("couldn't start listener: %s\n", err)
return 1
}
var server *dap.Server
disconnectChan := make(chan struct{})
server := dap.NewServer(&service.Config{
Listener: listener,
DisconnectChan: disconnectChan,
Debugger: debugger.Config{
Backend: backend,
Foreground: headless && tty == "",
DebugInfoDirectories: conf.DebugInfoDirectories,
CheckGoVersion: checkGoVersion,
TTY: tty,
},
CheckLocalConnUser: checkLocalConnUser,
})
defer server.Stop()

if dapConnect == "" {
listener, err := net.Listen("tcp", addr)
if err != nil {
fmt.Printf("couldn't start listener: %s\n", err)
return 1
}
server = dap.NewServer(&service.Config{
Listener: listener,
DisconnectChan: disconnectChan,
Debugger: debugger.Config{
polinasok marked this conversation as resolved.
Show resolved Hide resolved
Backend: backend,
Foreground: headless && tty == "",
DebugInfoDirectories: conf.DebugInfoDirectories,
CheckGoVersion: checkGoVersion,
TTY: tty,
},
CheckLocalConnUser: checkLocalConnUser,
})
} else { // reverse mode
hyangah marked this conversation as resolved.
Show resolved Hide resolved
headless = true // TODO(github.com/go-delve/delve/issues/2552): consider the same for the normal mode.
hyangah marked this conversation as resolved.
Show resolved Hide resolved

conn, err := net.Dial("tcp", dapConnect)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if err != nil {
fmt.Fprintf(os.Stderr, "Failed to connect to the DAP proxy server: %v\n", err)
hyangah marked this conversation as resolved.
Show resolved Hide resolved
return 1
}
server = dap.NewReverseServer(&service.Config{
DisconnectChan: disconnectChan,
Debugger: debugger.Config{
Backend: backend,
Foreground: headless && tty == "",
DebugInfoDirectories: conf.DebugInfoDirectories,
CheckGoVersion: checkGoVersion,
TTY: tty,
},
}, conn)
}
defer server.Stop()
server.Run()
waitForDisconnectSignal(disconnectChan)
return 0
Expand Down
43 changes: 43 additions & 0 deletions cmd/dlv/dlv_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"go/types"
"io"
"io/ioutil"
"net"
"os"
"os/exec"
"path/filepath"
Expand Down Expand Up @@ -644,6 +645,48 @@ func TestDap(t *testing.T) {
cmd.Wait()
}

// TestDapReverse verifies that a dap-reverse command can be started and shut down.
func TestDapReverse(t *testing.T) {
listener, err := net.Listen("tcp", ":0")
if err != nil {
t.Fatalf("cannot setup listener required for testing: %v", err)
}
defer listener.Close()

dlvbin, tmpdir := getDlvBin(t)
defer os.RemoveAll(tmpdir)

cmd := exec.Command(dlvbin, "dap", "--log-output=dap", "--log", "--connect", listener.Addr().String())
buf := &bytes.Buffer{}
cmd.Stdin = buf
cmd.Stdout = buf
assertNoError(cmd.Start(), t, "start reverse dap instance")

// Wait for the connection.
conn, err := listener.Accept()
if err != nil {
cmd.Process.Kill() // release the port
t.Fatalf("Failed to get connection: %v", err)
}
t.Log("dlv-reverse dialed in successfully")

client := daptest.NewClientWithConn(conn)
client.InitializeRequest()
client.ExpectInitializeResponse(t)

// Close the connection.
if err := conn.Close(); err != nil {
cmd.Process.Kill()
t.Fatalf("Failed to get connection: %v", err)
}

// Connection close should trigger dlv-reverse command's normal exit.
if err := cmd.Wait(); err != nil {
cmd.Process.Kill()
t.Fatalf("command failed: %v\n%s\n%v", err, buf.Bytes(), cmd.Process.Pid)
}
}

func TestTrace(t *testing.T) {
dlvbin, tmpdir := getDlvBin(t)
defer os.RemoveAll(tmpdir)
Expand Down
4 changes: 4 additions & 0 deletions service/dap/daptest/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ func NewClient(addr string) *Client {
if err != nil {
log.Fatal("dialing:", err)
}
return NewClientWithConn(conn)
}

func NewClientWithConn(conn net.Conn) *Client {
hyangah marked this conversation as resolved.
Show resolved Hide resolved
c := &Client{conn: conn, reader: bufio.NewReader(conn)}
c.seq = 1 // match VS Code numbering
return c
Expand Down
44 changes: 40 additions & 4 deletions service/dap/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ import (
// program termination and failed or closed client connection
// would also result in stopping this single-use server.
//
// The DAP server operates via the following goroutines:
// The DAP server initialized with NewServer
// operates via the following goroutines:
hyangah marked this conversation as resolved.
Show resolved Hide resolved
//
// (1) Main goroutine where the server is created via NewServer(),
// started via Run() and stopped via Stop(). Once the server is
Expand Down Expand Up @@ -87,11 +88,20 @@ import (
// They block on running debugger commands that are interrupted
// when halt is issued while stopping. At that point these goroutines
// wrap-up and exit.
//
// The DAP server set up using NewReverseDAPServer is a special
// DAP server, that is bound to a single net.Conn. Once the connection
// is closed, the server is stopped. Its Run is never called, but
hyangah marked this conversation as resolved.
Show resolved Hide resolved
// the NewReverseDAPServer immediately starts serveDAPCodec on the
// net.Conn.
type Server struct {
// config is all the information necessary to start the debugger and server.
config *service.Config
// listener is used to accept the client connection.
listener net.Listener
// inReverseMode is true if this server operates in reverse mode
// where listener is nil.
inReverseMode bool
// stopTriggered is closed when the server is Stop()-ed.
stopTriggered chan struct{}
// reader is used to read requests from the connection.
Expand Down Expand Up @@ -197,9 +207,25 @@ const (
// disconnects or requests shutdown. Once config.DisconnectChan is closed,
// Server.Stop() must be called to shutdown this single-user server.
func NewServer(config *service.Config) *Server {
return newServer(config, nil, false)
}

// NewReverseServer creates a special DAP server that operates in reverse mode.
// Reverse DAP server is not actually running a TCP server listening on a port,
// but communicates with the provided net.Conn only for a single debug session.
func NewReverseServer(config *service.Config, conn net.Conn) *Server {
hyangah marked this conversation as resolved.
Show resolved Hide resolved
return newServer(config, conn, true)
}

func newServer(config *service.Config, conn net.Conn, inReverseMode bool) *Server {
hyangah marked this conversation as resolved.
Show resolved Hide resolved
logger := logflags.DAPLogger()
logflags.WriteDAPListeningMessage(config.Listener.Addr().String())
logger.Debug("DAP server pid = ", os.Getpid())
if !inReverseMode {
logflags.WriteDAPListeningMessage(config.Listener.Addr().String())
logger.Debug("DAP server pid = ", os.Getpid())
} else {
logger.Debug("Reverse DAP server pid = ", os.Getpid())
}

return &Server{
config: config,
listener: config.Listener,
Expand All @@ -209,6 +235,8 @@ func NewServer(config *service.Config) *Server {
variableHandles: newVariablesHandlesMap(),
args: defaultArgs,
exceptionErr: nil,
inReverseMode: inReverseMode,
hyangah marked this conversation as resolved.
Show resolved Hide resolved
conn: conn,
}
}

Expand Down Expand Up @@ -265,7 +293,10 @@ func (s *Server) setLaunchAttachArgs(request dap.LaunchAttachRequest) error {
func (s *Server) Stop() {
s.log.Debug("DAP server stopping...")
close(s.stopTriggered)
_ = s.listener.Close()

if !s.inReverseMode {
_ = s.listener.Close()
}

s.mu.Lock()
defer s.mu.Unlock()
Expand Down Expand Up @@ -326,6 +357,11 @@ func (s *Server) triggerServerStop() {
// TODO(polina): allow new client connections for new debug sessions,
// so the editor needs to launch delve only once?
func (s *Server) Run() {
if s.inReverseMode {
go s.serveDAPCodec()
hyangah marked this conversation as resolved.
Show resolved Hide resolved
return
polinasok marked this conversation as resolved.
Show resolved Hide resolved
}

go func() {
conn, err := s.listener.Accept() // listener is closed in Stop()
if err != nil {
Expand Down