Skip to content

Debug adapter protocol client implementation and bridge mode for IDE - #78

Draft
David Negstad (danegsta) wants to merge 31 commits into
mainfrom
dev/danegsta/dap
Draft

Debug adapter protocol client implementation and bridge mode for IDE#78
David Negstad (danegsta) wants to merge 31 commits into
mainfrom
dev/danegsta/dap

Conversation

@danegsta

Copy link
Copy Markdown
Member

Implements a debug adapter protocol (DAP) client and support for a new debug adapter protocol bridge based IDE debugging mode.

Opening this as a draft because it's both very large and I want to spend more time on the Aspire side of the bridge to make sure things are fully stable before I mark it ready for review.

@karolz-msKarol Zadora-Przylecki (karolz-ms) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Big PR, so will be sending my feedback in "chunks" -- hope this helps!

(this is the first one)

Comment threadpkg/process/process_handle_test.go
Comment threadpkg/process/process_handle.go Outdated
Comment threadinternal/networking/unix_socket.go Outdated
Comment threadinternal/networking/unix_socket.go Outdated
Comment threadinternal/networking/unix_socket.go Outdated

l.closed = true

closeErr := l.listener.Close()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Release lock before calling Close()

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I implemented the retained error, which means we have to hold the lock until we've actually resolved the error value.

Comment threadinternal/networking/unix_socket.go Outdated
Comment threadinternal/notifications/notifications.go
Comment threadinternal/networking/unix_socket_test.go Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Initial PR review part 2

Comment threadinternal/networking/unix_socket.go Outdated
Comment threadinternal/networking/unix_socket_test.go Outdated
t.Parallel()
rootDir := shortTempDir(t)

listener, createErr := NewSecureSocketListener(rootDir, "afc-")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The test that we should really have is the following:

  1. Create a new socket listener and do Accept() (blocks).
  2. Launch 10 different goroutines and make them race to call Close() on the listener.
  3. Verify that Accept() returns with errClosed (a distinct error that allows the caller to differentiate between "somebody closed be because it is time to shut down" vs another, unexpected error).
  4. Verify that all calls to Close() returned with no error.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Comments on the DAP implmentation, part 1

Comment threadinternal/dap/adapter_launcher.go Outdated
Comment threadinternal/dap/adapter_launcher.go Outdated
Comment threadinternal/dap/adapter_launcher.go Outdated
Comment threadinternal/dap/adapter_launcher.go
Comment threadinternal/dap/adapter_launcher.go Outdated
Comment threadinternal/dap/bridge.go Outdated
Comment threadinternal/dap/bridge.go Outdated
Comment on lines +207 to +217
// If the adapter did not send a TerminatedEvent, synthesize one for the IDE.
// Also send an error OutputEvent if we exited due to a transport error.
terminated := b.terminatedEventSeen.Load()

if !terminated {
if loopErr != nil && !errors.Is(loopErr, io.EOF) && !errors.Is(loopErr, context.Canceled) {
b.sendErrorToIDE(fmt.Sprintf("Debug session ended unexpectedly: %v", loopErr))
} else {
b.sendTerminatedToIDE()
}
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I would prefer if the forwardAdapterToIDE goroutine was "told" to do this (e.g. via a separate channel). This eliminates the race between the two goroutines with regards to reading/writing terminatedEventSeen and also ensures that a single goroutine is writing to the IDE socket.

To be clear, having multiple goroutines writing to the same socket is not strictly forbidden, but adds another degree of asynchrony.

Comment threadinternal/dap/bridge.go
Comment threadinternal/dap/bridge.go
Comment threadinternal/dap/bridge.go Outdated
func (b *DapBridge) handleOutputEvent(event *dap.OutputEvent) {
// Only capture output if runInTerminal wasn't used
// (if runInTerminal was used, we capture directly from the process)
if !b.runInTerminalUsed.Load() && b.config.OutputHandler != nil {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Seems like runInTerminalUsed is only used by the adapter --> IDE goroutine, so it can probably be just a regular bool flag.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I'm mostly leaving this as an atomic bool out of an abundance of caution.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

DAP bridge review, part 2

Comment on lines +58 to +60
type HandshakeReader struct {
conn net.Conn
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is there a particular reason to define HandshakeReader this way as oppose do

Suggested change
typeHandshakeReaderstruct {
conn net.Conn
}
typeHandshakeReader net.Conn

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

No specific reason

Comment on lines +109 to +111
type HandshakeWriter struct {
conn net.Conn
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same question as for HandshakeReader

Suggested change
typeHandshakeWriterstruct {
conn net.Conn
}
typeHandshakeWriter net.Conn

Comment threadinternal/dap/bridge_integration_test.go
Comment threadinternal/dap/bridge_manager.go Outdated
Comment threadinternal/dap/bridge_manager.go Outdated
Comment threadinternal/dap/bridge_manager.go
Comment threadinternal/dap/bridge_manager.go Outdated
Comment threadinternal/dap/bridge_manager.go Outdated
Comment threadinternal/dap/bridge_manager.go Outdated
Comment threadinternal/dap/bridge_manager.go Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Finished initial review. This is a fantastic and impressive change!

Comment threadinternal/dap/transport.go Outdated
Comment threadinternal/dap/transport.go
Comment threadMakefile Outdated
Comment threadinternal/dap/transport.go Outdated
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@danegsta@karolz-ms