Repository navigation
Continue for all threads #214
Description
Activity
Yes, today DAP does not support an "all threads" mode for the thread oriented requests like
continue,next,stepIn,stepOut,reverseContinue, andstepBack.vadimcn you said:
VSCode always sends a valid thread id, even when I click
▶️ in the top debugger toolbar.I do not understand what you mean by "the top debugger toolbar" but are you saying that VS Code should use a "Continue all threads" in that context, which today is impossible because it is not supported by DAP?
I do not understand what you mean by "the top debugger toolbar"
I mean the main debugger toolbar that appears at the top of VSCode window when debugging:
My expectation is that when I click the "play" button on that toolbar, all threads should be resumed, not just the currently selected one.
"Continue all threads" in that context, which today is impossible because it is not supported by DAP?
Yes. Or, rather, I'm saying that there should be some way to unpause all threads. When the debug adapter supports per-thread control, unpausing all of them one by one gets rather tedious. Try this with e.g Python extension's debugger.
Note that there's already
ContinuedEvent.allThreadsContinued. So in practice, the adapter can treat "continue" requests (and stepping etc) as resuming all threads, if that's the best it can do.In fact, we make use of this feature of DAP in the Python debug adapter - it's configurable to continue one thread vs all threads on step. This was required to use the adapter with Visual Studio due to this:
Visual Studio requires all threads to enter and leave break mode at the same time. Stepping, stopping, or continuing a single thread is not supported.
Furthermore, even for VSCode, we resume all threads on step by default - it turned out that most users find it less confusing in practice.
We also have a protocol extension in place whereby specifying
"threadId": "*"can be used to explicitly continue all threads.Note that there's already ContinuedEvent.allThreadsContinued
Sure, but this is adapter's choice, which user cannot influence. This seems sub-optimal...
The reason I am raising this, is that I've looked into implementing stopping/resuming a specific thread for my debug adapter, because this is useful every once in a while. However, this leads to degradation of the common use-case, so I've decided it isn't worth it. I suppose having this as a config setting is an option, but discoverability would be low.
Furthermore, even for VSCode, we resume all threads on step by default - it turned out that most users find it less confusing in practice.
So what's the purpose of having per-thread execution control buttons in VSCode? I realize this is DAP repo, not VSCode's, but still, there must have been some internal discussion between the teams?
We also have a protocol extension in place whereby specifying "threadId": "*" can be used to explicitly continue all threads.
But which DAP clients support it? VSCode doesn't (unless there's some magic option that enables this?)
In short, I would appreciate some guidance on how this is supposed to be implemented. Thanks!
To clarify, what I wrote above pertains specifically to the Python debug adapter, which is its own thing separate from VSCode proper. Other debug adapters do things differently.
Pavel Minaev (@int19h) vadimcn thanks a lot for your clarifications.
I definitively see the need for improving this both on the Debug Adapter Protocol as well as in VS Code.
So let's start with DAP: what are the options?
It would be nice if we could just make the
threadIdproperty optional and introduce the new semantics "missing threadId means 'all threads'". But unfortunately this is not possible because an optionalthreadIdwould break all debug adapters that are relying onthreadIdbeing mandatory.Since
threadIdis of typenumber, using a wildcard pattern '*' isn't possible either.So I think the only option we have is to introduce a new boolean property
allThreadson thecontinue,next,stepIn,stepOut,reverseContinue, andstepBackrequests with the semantics: "ifallThreadsis true, thethreadIdproperty is ignored and the request applies to all threads. In addition a new debug adapter capabilitysupportsAllThreadsindicates to the client that a debug adapter understands theallThreadsproperty.What do you think?
But unfortunately this is not possible because an optional threadId would break all debug adapters that are relying on threadId being mandatory.
If you are adding a new capability, backwards compatibility with existing adapters shouldn't be a problem?
Regarding
next,stepIn,stepOutandstepBack: IMO, "step all threads to the next line" does not make much sense; at least I had never needed that. There's always a current thread that you want to step.
On the other hand, some debuggers do make a distinction between stepping the current thread while allowing others to run freely and stepping the current thread with all other frozen. The latter operation is somewhat obscure though, so maybe DAP doesn't need to support this case.With respect to stepping, yes, the difference is exactly that - whether other threads are frozen or not during the stepdap. Our takeaway from the DAP spec was that stepping while other threads are frozen is the intended behavior. But, in practice, this leads to issues when the stepped thread needs to acquire a lock that is held by one of the frozen threads - and this can be very non-obvious to the user if the lock is somewhere in library code that they are stepping over. If the intent was to unfreeze all threads during the step, I think that might be worth clarifying in the spec.
(Aside from that, there's the issue of VS DAP client being more broadly broken here, in that it expects all threads to unfreeze on step, but does not expect to get "continue" events for any threads other than the one it requested a step on. Conversely, VSCode, and any other properly conforming DAP client, expects "continue" events for all threads that were resumed. Thus, as things stand, there's no way to implement multi-threaded stepping portably across VS and VSCode without relying on "clientID" to special-case VS. This is a VS bug, of course, but they can't easily fix it because all-threads-run during stepping has been a fundamental assumption for VS debugger infrastructure for a very long time. I hope that, if DAP gets a clear way to indicate this semantics in the step request, VS would just adopt it, and we can drop any special treatment in adapters.)
My concern with adding a new flag to the request which directs the adapter to ignore
threadId, is that existing adapters will quietly ignore the flag instead, and thus the behavior will not be what the client might have expected - but with no diagnostics. OTOH ifthreadIdis made optional, then any existing adapter that doesn't understand it will fail fast, and the error message should make the problem clear. Since client is obligated to check capabilities before issuing requests that depend on them, an adapter that doesn't advertisesupportsAllThreadsshould never get a request withoutthreadId(and if it does, then it's a client bug - which, again, should be easy to diagnose from error messages).Reacted by Ben Jacksonvadimcn Pavel Minaev (@int19h) thanks for your feedback!
vadimcn you said:
If you are adding a new capability, backwards compatibility with existing adapters shouldn't be a problem?
A new boolean capability cannot be used in JSON-schema to express that a property is either optional or mandatory (based on the dynamic value of a boolean), and the same holds for the generated Typescript or any other language.
Pavel Minaev (@int19h) you said:
My concern with adding a new flag to the request which directs the adapter to ignore
threadId, is that existing adapters will quietly ignore the flag instead,..Existing adapters will only receive the new flag
allThreadsif they opt into the feature by explicitly returning a value oftruefor thesupportsAllThreadscapability. If they are not opting into this, then there will be noallThreadsflag and the client will know that the debug adapter cannot deal with "all threads.... if threadId is made optional, then any existing adapter that doesn't understand it will fail fast...
Sorry, but so far we have evolved the DAP without breaking any existing debug adapter. I think we should not change this rule without a pressing need. In addition, making "threadId" optional will break existing extensions syntactically even if the client always provides a "threadId".
Vadim Patsalo (@vadim) you said:
Regarding next, stepIn, stepOut and stepBack: IMO, "step all threads to the next line" does not make much sense; at least I had never needed that. There's always a current thread that you want to step.
Pavel Minaev (@int19h) has a different view on this:
Furthermore, even for VSCode, we resume all threads on step by default - it turned out that most users find it less confusing in practice.
7 remaining items
Here is what I did:
- Clarified the description of all execution control requests (
continue,next,stepIn,stepOut,stepBack,reverseContinue) - Added a new capability
supportsSingleThreadExecutionRequeststo indicate that the execution control requests support thesingleThreadproperty. - Added a new optional
singleThreadproperty to all execution control requests.
Reacted by Pavel Minaev- Clarified the description of all execution control requests (
What is implemented in practice (aka "useful semantics"):
"step" requests step the current thread while allowing others threads to run freely by resuming them
This seems like odd (default) behaviour to me. If I've paused all threads and I start stepping through one of them, having the other all start running (and potentially generate output, or interfere with what I'm debugging) is not what I'd expect.
Does VS Code now assume all threads are resumed when I step? If so, this seems like a bit of a breaking change for all DAs following the original spec (which IMO seems much more obvious behaviour than this) until we ship updates :(
Although, in my testing when I click Step or Resume for one thread, VS Code is still showing the others as "Paused on breakpoint". So it seems to still work as I'd expect, although that doesn't seem to match the new descriptions 🤔
Danny Tuppeny (@DanTup) I agree that it is seemingly more obvious behavior to resume only the thread being stepped; it's just that it turns out to be unworkable in practice.
Consider a simple example with native code: two threads, the main one, and one running in the background doing some processing. You set a breakpoint to debug the main thread. When it's hit, both threads are paused; the main one is on the breakpoint, while the background thread was in the middle of a
malloccall. The background thread is currently holding the heap lock. If you try to step in the main thread, and the code that you stepped over tries to do amallocitself (likely without you even knowing that it will, in case of library code), try to acquire the heap lock, and stall there. The heap lock is never going to be released since the background thread that's holding it is still frozen. Effectively, your step never ends - and you don't have any clue as to why!If you know that it's because the other thread is holding a lock, you can manually unfreeze it. But in practice, you'd usually have a thread pool running numerous threads - good luck guessing which one of them holds the necessary lock. Worse yet, there may well be more than one lock involved.
This is by no means exclusive to native code, either - any locking can trigger this scenario quite easily.
Consequently, most debuggers out there resume all threads during stepping, at least by default (some, like e.g. gdb, have different modes).
Consider a simple example with native code: two threads
I'm not debating that it makes sense in some cases for many languages in many situations. But I don't think it's the obvious thing to do (either logically as a user, or based on the API parameters). I can't say for certain, but I'm fairly sure when I was doing .NET, clicking Step in the debugger wouldn't resume other threads? If I want to resume the other threads, I could easily do it with the old behaviour - but what if I don't want to do that with the new behaviour?
The thing that frustated me a little more though is that this seems like a breaking change for people that were doing the thing the API seemed to describe (despite comments above about not breaking existing DAs) - why couldn't the new behaviour have been opt-in with the new capability?
I'm in the process of moving my DA from the VS Code extension into the Dart SDKs (so it can be used by other editors), where the release cycles will be significantly longer. Any breaking changes made will take a lot longer to respond to, so seeing them being made seemingly unnecessarily and with little notice (the first I saw was in the VS Code release notes) does slightly concern me 😞
The .NET debugger in both VS and VSCode resumes all threads during stepping. FWIW I also thought that it didn't work like that originally, and was quite surprised when we ran into this issue and went to look at what the established practice is!
As far as back-compat, from what I can see, the changes to the spec don't break it because the adapter still has to communicate which threads have been resumed (and thus, by omission, which haven't) via the "continued" event. Unless I'm missing something, and the spec now mandates not sending that event for threads other than the target of "next" - which would indeed be a breaking change for all existing adapters, even those that already resume all threads. Andre Weinand (@weinand), can you clarify this?
Pavel Minaev (@int19h) no, the spec does not change anything with respect to sending events. And we do not plan to change anything in VS Code as a result of the clarification.
The clarification just tries to make the spec better match what real debug adapters are actually implementing.The .NET debugger in both VS and VSCode resumes all threads during stepping.
Oh, that does surprise me. Perhaps when debugging I didn't often have multiple threads paused (eg. just hit breakpoints on the thread I was debugging) and didn't notice.
the changes to the spec don't break it because the adapter still has to communicate which threads have been resumed (and thus, by omission, which haven't) via the "continued" event.
Oh, I misunderstood and thought the editor would now assume the other threads are resumed. If that's not the case, what's the purpose of the new capability and changing the default for
allThreadsContinued? What's the difference between me doing nothing (continuing to only resume the one thread that was requested and not sendingallThreadsContinued), versus signalling the new capability and sendingallThreadsContinued=false?My understanding is that the new capability is for the client to be able to request "step just this thread" specifically, and know that this would not involve any other threads (if the capability is supported). Whereas the default is that if you request to step one thread, it may or may not step other threads.
Pavel Minaev (@int19h) thanks a lot for providing the perfect answer to Dan's question - I had planned something similar but forgot to reply over the weekend...
Thanks for the clarification, I think this makes sense.
Although I think it may be worth calling out in the spec that DAs that do not set
supportsSingleThreadExecutionRequestsare not guaranteed to behave a specific way, as it seems easy to assume that not setting this means the DA won't do single-thread stepping when in reality it might, and it seems there's no way to advertise if a DA will always behave in a particular way without supporting both.(I don't want to support both in Dart without seeing what it will change in the debugging UIs, because if resuming all threads becomes the default/easiest action, I don't think that's what users would expect)
- added a commit that references this issue
on Jan 13, 2025
The▶️ in the top debugger toolbar.
continuerequest hasthreadIdattribute to specify which thread should continue execution. However, if this were implemented as spec'ed, how would the user signal the intent to continue running all threads?Empirically, VSCode always sends a valid thread id, even when I click