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.

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

Merged
merged 6 commits into from
Mar 10, 2020

Conversation

polinasok
Copy link
Collaborator

@polinasok polinasok commented Mar 5, 2020

With this change, VS Code no longer gets stuck when paired with the new dlv-dap server and can use it to run simple debug session sequences with stop/continue on entry, set breakpoint, continue, and disconnect (although additional features are still needed for full integration with the UI).

This change also:
-- Updates the test client and unittest to start seq numbers from 1 to match VS Code
-- Expands TestStopOnEntry to cover the complete sequence of messages observed with VS Code with this most basic debug session scenario. Adds matching TestContinueOnEntry test.
-- Fixes TestSetBreakpoint not to send a redundant continue request.
-- Adds tests for when the threads request are issued during launch, mid-session and after termination.

Updates #1515

@@ -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)
Copy link
Contributor

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

Copy link
Collaborator Author

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.

Copy link
Contributor

@eliben eliben Mar 6, 2020

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.

Copy link
Contributor

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)


threads := make([]dap.Thread, len(gs))
if len(threads) == 0 {
threads = []dap.Thread{{Id: 1, Name: "Dummy"}}
Copy link
Contributor

Choose a reason for hiding this comment

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

(out of curiosity) why is this dummy one necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a comment in the code. This "magic" definitely deserves one.
I learned this based on the implementation of the TypeScript adaptor implementation in vscode-go, which was fixed a while back to do this. See the discussion here: microsoft/vscode-go#2126 (comment)

Copy link
Member

Choose a reason for hiding this comment

The 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 GoroutineID == 0 and set dap.Thread.Id = -thread.ID (i.e. os threads have negative IDs, goroutines have positive IDs).

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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:

  • GoroutineID == -1 the currently selected goroutine or the current thread if it isn't running a goroutine
  • GoroutineID == 0 the current thread if it isn't running a goroutine (an error otherwise)
  • GoroutineID > 0 the specified goroutine if it can be found

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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)
and if !ok, t.Errorf, otherwise return r.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ditto

} else {
for i, g := range gs {
threads[i].Id = g.ID
if g.UserCurrentLoc.Function != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

if fn := g.UserCurrentLoc.Function; fn != nil {
threads[i].Name = fn.Name()
} else {
threads[i].Name = fmt.Sprintf(..)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I made the change, but I am not sure it is making things better given that fn is only used in the if-part. And now it is not so obvious that the if and the else parts rely on the same parent struct, just different subfields. Is this a best practice of some sort that just looks odd to my untrained newbie eye?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wanted to shorten the line :-) If you desire to make it clear that the if/else share the same g.UserCurrentLoc,

if loc := g.UserCurrentLoc; loc.Function != nil {
   threads[i].Name = loc.Function.Name()
} else {
   threads[i].Name = fmt.Sprintf("%s@%d", loc.File, loc.Line)
}

t.Errorf("\ngot %#v\nwant len(Threads)>1", tResp.Body.Threads)
}
// TODO(polina): can we reliably test for these values?
wantMain := dap.Thread{Id: 1, Name: "main.Increment"}
Copy link
Contributor

Choose a reason for hiding this comment

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

is it really true that main.Increment always has thread id=1?
Can we relax the test case and check just whether there are threads with name=main.Increment, runtime.gopark? Or, is the id mapping important?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Other delve tests rely on this id, so I assumed this would be ok. Note that my runtime check is quite relaxed, only testing for the prefix. I use the thread struct in the log only for illustration purposes. The TODO comment is there to prompt the delve owners to chime in as well. I also emailed Austin earlier today for additional input on what I can safely rely on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Austin confirmed that id 1 (but not others) can be relied on. I am still clarifying if the check for "runtime" prefix is ok as well.

Copy link
Member

Choose a reason for hiding this comment

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

It's probably fine, other goroutines are only going to do GC work (if they are not parked) which will all happen inside the runtime (afaik).

Copy link
Member

@aarzilli aarzilli left a comment

Choose a reason for hiding this comment

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

lgtm

s.sendErrorResponse(request.Request, UnableToDisplayThreads, "Unable to display threads", "debugger is nil")
return
}
gs, _, err := s.debugger.Goroutines(0, 0)
Copy link
Member

@aarzilli aarzilli Mar 6, 2020

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. Is this the only case? Then it is the same case where I am forced to send back a dummy thread when stopping on entry, isn't it? So if the debug state has nil, won't the list of goroutines be empty as well?

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).

They use the selected goroutine id to decide which goroutine to highlight as stopped on in the UI by sticking it into the stopped event. It doesn't seem that id 0 would be of much help. What does that represent anyway? I am still not quite clear about your earlier suggestion.

It won't help you with that, it would help you if you used it to request local variables, stack traces or evaluate expressions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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 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?

The proper way to do it would be to leave most fields empty and copy the current location from the CurrentThread field.

That doesn't result in anything useful. No stacktraces or variables are requested/displayed with id 0.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

Please see microsoft/debug-adapter-protocol#159.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks.

Copy link
Member

@derekparker derekparker left a comment

Choose a reason for hiding this comment

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

Overall looks good, just one nit.

One other question though is does the current VSCode adapter not distinguish between actual OS threads and goroutines? I suppose it might not need to and there really isn't a generic DAP way of requesting something Go specific such as Goroutines.

service/dap/server.go Outdated Show resolved Hide resolved
@polinasok
Copy link
Collaborator Author

Yes, you are right. DAP doesn't distinguish between OS and user threads. And it doesn't have any special support for goroutines. My implementation follows what vscode-go does here:
https://github.com/microsoft/vscode-go/blob/master/src/debugAdapter/goDebug.ts#L906
You can read more about threads requests here:
https://microsoft.github.io/debug-adapter-protocol/specification#Requests_Threads
https://microsoft.github.io/debug-adapter-protocol/specification#Types_Thread
https://microsoft.github.io/debug-adapter-protocol/overview (Supporting Threads section)

@derekparker derekparker merged commit 5613cf1 into go-delve:master Mar 10, 2020
@polinasok polinasok deleted the threads_request branch March 18, 2020 17:12
cgxxv pushed a commit to cgxxv/delve that referenced this pull request Mar 25, 2022
* Add support for threads request

* Address review comments

* Relax threads test condition

* Address review comments

* Clean up unnecessary newline

* Respond to review comment

Co-authored-by: Polina Sokolova <polinasok@users.noreply.github.com>
abner-chenc pushed a commit to loongson/delve that referenced this pull request Mar 1, 2024
* Add support for threads request

* Address review comments

* Relax threads test condition

* Address review comments

* Clean up unnecessary newline

* Respond to review comment

Co-authored-by: Polina Sokolova <polinasok@users.noreply.github.com>
Sign up for free to 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.

5 participants