-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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.
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
service/dap: Add support for threads request #1914
Changes from 4 commits
a8a8282
51ee84f
32d3854
ff2bd92
734f160
66971b6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,6 @@ package daptest | |
|
||
import ( | ||
"bufio" | ||
"encoding/json" | ||
"fmt" | ||
"log" | ||
"net" | ||
|
@@ -34,6 +33,7 @@ func NewClient(addr string) *Client { | |
log.Fatal("dialing:", err) | ||
} | ||
c := &Client{conn: conn, reader: bufio.NewReader(conn)} | ||
c.seq = 1 // match VS Code numbering | ||
return c | ||
} | ||
|
||
|
@@ -43,8 +43,6 @@ func (c *Client) Close() { | |
} | ||
|
||
func (c *Client) send(request dap.Message) { | ||
jsonmsg, _ := json.Marshal(request) | ||
fmt.Println("[client -> server]", string(jsonmsg)) | ||
dap.WriteProtocolMessage(c.conn, request) | ||
} | ||
|
||
|
@@ -120,6 +118,16 @@ func (c *Client) ExpectConfigurationDoneResponse(t *testing.T) *dap.Configuratio | |
return c.expectReadProtocolMessage(t).(*dap.ConfigurationDoneResponse) | ||
} | ||
|
||
func (c *Client) ExpectThreadsResponse(t *testing.T) *dap.ThreadsResponse { | ||
t.Helper() | ||
return c.expectReadProtocolMessage(t).(*dap.ThreadsResponse) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if expectReadProtoMessage returns a nil or a message that can't be *dap.ThreadsResponse, this type assertion will trigger runtime panic. In order to avoid, you can use r, ok := c.expectReadProtocolMessage(t).(*dap.ThreadsResponse) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
} | ||
|
||
func (c *Client) ExpectStackTraceResponse(t *testing.T) *dap.StackTraceResponse { | ||
t.Helper() | ||
return c.expectReadProtocolMessage(t).(*dap.StackTraceResponse) | ||
} | ||
|
||
// InitializeRequest sends an 'initialize' request. | ||
func (c *Client) InitializeRequest() { | ||
request := &dap.InitializeRequest{Request: *c.newRequest("initialize")} | ||
|
@@ -199,6 +207,18 @@ func (c *Client) ContinueRequest(thread int) { | |
c.send(request) | ||
} | ||
|
||
// ThreadsRequest sends a 'threads' request. | ||
func (c *Client) ThreadsRequest() { | ||
request := &dap.ThreadsRequest{Request: *c.newRequest("threads")} | ||
c.send(request) | ||
} | ||
|
||
// StackTraceRequest sends a 'stackTrace' request. | ||
func (c *Client) StackTraceRequest() { | ||
request := &dap.StackTraceRequest{Request: *c.newRequest("stackTrace")} | ||
c.send(request) | ||
} | ||
|
||
// UnknownRequest triggers dap.DecodeProtocolMessageFieldError. | ||
func (c *Client) UnknownRequest() { | ||
request := c.newRequest("unknown") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -242,7 +242,7 @@ func (s *Server) handleRequest(request dap.Message) { | |
case *dap.SourceRequest: | ||
s.sendUnsupportedErrorResponse(request.Request) | ||
case *dap.ThreadsRequest: | ||
s.sendUnsupportedErrorResponse(request.Request) | ||
s.onThreadsRequest(request) | ||
case *dap.TerminateThreadsRequest: | ||
s.sendUnsupportedErrorResponse(request.Request) | ||
case *dap.EvaluateRequest: | ||
|
@@ -451,6 +451,50 @@ func (s *Server) onContinueRequest(request *dap.ContinueRequest) { | |
s.doContinue() | ||
} | ||
|
||
func (s *Server) onThreadsRequest(request *dap.ThreadsRequest) { | ||
if s.debugger == nil { | ||
s.sendErrorResponse(request.Request, UnableToDisplayThreads, "Unable to display threads", "debugger is nil") | ||
return | ||
} | ||
gs, _, err := s.debugger.Goroutines(0, 0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have any contacts on the VSCode side of things? This API is very unfortunate, it works ok for actual threads, which are bound to be few in number, but there could be a lot of goroutines and requesting all of them after every user action will slow down debugging a lot. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an interesting point. I will reach out to my contacts for additional input. They do make a lot of these requests and the current adaptor translates them into the get-all delve rpc call. And what is even more unfortunate is that I see many back-to-back dups in the logs with the exact same responses. Is your concern that the debugger will be busy dealing with each one and unable to respond to other requests or that sending the long list over the connection would add too much latency? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both. Also that the UI will be inefficient in handling a long list of goroutines and add further latency. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The overview documentation , "Whenever the generic debugger receives a stopped or a thread event, the development tool requests all threads that exist at that point in time. Thread events are optional, but a debug adapter can send them to force the development tool to update the threads UI dynamically even when not in a stopped state. If a debug adapter decides not to emit Thread events, the thread UI in the development tool will only update if a stopped event is received." Do you therefore recommend that I not implement the threads events, not even in the case where the program does not stop on entry, so no threads are requested and displayed then until another stop is reached? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, if you were to design a custom API for go for this, what would you do? Would you only allow calls upon a user request instead of automatically on any stop? Or try to differentiate between user-defined goroutines, runtime goroutines and any hidden library goroutines and limit the list by default to only the ones the user defined and hence cares about? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No, this will happen anytime we can't read the current goroutine from TLS. Another example is if cgo starts a thread that only executed C code and that thread gets selected as the current thread (either because of a breakpoint, a signal or because of a manual stop).
It won't help you with that, it would help you if you used it to request local variables, stack traces or evaluate expressions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The id in the stopped event not only helps the UI mark which thread is paused, but is also used in subsequent stack trace and variable requests to reflect that state of things in the UI as well. What exactly does goroutine id 0 represent? My understanding was that goroutines are numbered from 1 up. I see no currentGoroutine set when I use pause (which results in halt rpc). In that case, the code gets a list of goroutines and uses goroutine[0], which as far as I can tell is always id=1. The code expects the entire goroutine record, but I hacked it real fast and just put 0 for id with the rest of the fields coming from goroutine[0]. Is that what you meant? That doesn't result in anything useful. No stacktraces or variables are requested/displayed with id 0. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Goroutine id 0 is used for a series of special things in the go runtime, we're also treating it specially by allowing it to represent the current thread when the current thread doesn't have an associated goroutine.
The proper way to do it would be to leave most fields empty and copy the current location from the CurrentThread field.
This could be a bug in delve but it's hard to say because what happens when a pause is requested is somewhat random and depends on the program being run as well as the operating system. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Please see microsoft/debug-adapter-protocol#159. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. |
||
if err != nil { | ||
switch err.(type) { | ||
case *proc.ErrProcessExited: | ||
// If the program exits very quickly, the initial threads request will complete after it has exited. | ||
// A TerminatedEvent has already been sent. Ignore the err returned in this case. | ||
s.send(&dap.ThreadsResponse{Response: *newResponse(request.Request)}) | ||
default: | ||
s.sendErrorResponse(request.Request, UnableToDisplayThreads, "Unable to display threads", err.Error()) | ||
} | ||
return | ||
} | ||
|
||
threads := make([]dap.Thread, len(gs)) | ||
if len(threads) == 0 { | ||
// Depending on the debug session stage, goroutines information | ||
// might not be available. However, the DAP spec states that | ||
// "even if a debug adapter does not support multiple threads, | ||
// it must implement the threads request and return a single | ||
// (dummy) thread". | ||
threads = []dap.Thread{{Id: 1, Name: "Dummy"}} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (out of curiosity) why is this dummy one necessary? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a comment in the code. This "magic" definitely deserves one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm ok with the PR as is, but one way of dealing with this and also with the fact that there's no distinction between threads and goroutines is to loop through debugger.Threads() and add every thread that has There's always going to be at least one OS thread so the dummy response wouldn't be needed anymore (however this would mean that negative thread IDs have to be handled throughout the API). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For now my goal was to keep feature parity with the existing implementation, but we can definitely improve things going forward. I can add this to my personal TODO list or we can file an issue in this repo to investigate this further. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was about to file an issue for this, but have a follow-up question. A StackTrace request will be issued for each reported thread. For the dummy thread, we end up with an unknown goroutine error, so the editor displays "unable to provide stack trace" for it. What would be the behavior for the stacktrace behavior for OS threads? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I understand the question. If you're asking how the stacktrace api interprets its GoroutineID parameter then you can look at proc.FindGoroutine in pkg/proc/variables.go. Basically:
|
||
} else { | ||
for i, g := range gs { | ||
threads[i].Id = g.ID | ||
if fn := g.UserCurrentLoc.Function; fn != nil { | ||
threads[i].Name = fn.Name() | ||
} else { | ||
threads[i].Name = fmt.Sprintf("%s@%d", g.UserCurrentLoc.File, g.UserCurrentLoc.Line) | ||
} | ||
|
||
derekparker marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
response := &dap.ThreadsResponse{ | ||
Response: *newResponse(request.Request), | ||
Body: dap.ThreadsResponseBody{Threads: threads}, | ||
} | ||
s.send(response) | ||
} | ||
|
||
func (s *Server) sendErrorResponse(request dap.Request, id int, summary string, details string) { | ||
er := &dap.ErrorResponse{} | ||
er.Type = "response" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi - if this type assertion fails (because c.expectReadProtoMessage returns a nil or something other type that can't be type asserted as *dap.ThreadsResponse, this can cause a runtime panic. In testing, panic is not that great. Something like this will avoid runtime panic.
v, ok := c.expectReadProtoMessage(t).(*dap.ThreadsResponse)
if !ok {
t.Errorf(...)
}
return v
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new helpers are consistent with the other ones in this file. Those were introduced by @eliben. See the explanation here where I made the same code review comments about panics: https://github.com/eliben/delve/pull/1#discussion_r381480812.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally, the idea was to keep these helper functions short because we abstracted them out and added a lot of repetitive code. A panic in a test will fail the test and show where it failed, so it seemed acceptable. FWIW these specific failures/panics are expected to be extremely rare.
Adding a non-panicking type assertion will replace panics by errors, but it will also increase the size of the repetitive code considerably. This has to be repeated for every message type (including the error message added in
t.Errorf
), and code-generation seemed like an overkill for these test helpers.That's the tradeoff that seemed reasonable to me at the time of writing. I don't have a strong feeling about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification. Since this is only for test, maybe ok.
I personally don't trust panic handling in test (e.g. golang.org' issue/37555 as one of the recent failures)