StoppedEvent should indicate thread focus

[roblourens]

StoppedEvent should indicate whether this thread should get focus

Example: some thread hits a breakpoint, and all other threads stop. The later StoppedEvents might steal focus from the important thread, which was the first StoppedEvent

export interface StoppedEvent extends Event {
    body: {
        // ...
        threadCausedFocus?: boolean;
    }
}

cc @mostafaeweda @ebluestein

[ebluestein]

Yep totally! The goal here would be to be able to distinguish between the thread that is responsible for stopping the target (hit a bp, threw an exception, etc) and others that stopped because one thread hit a bp and we have a "stop one, stop all" policy in the debugger.

I'd like to make it explicit because the back end can't necessarily guarantee that the first StoppedEvent the UX sees is from the thread that hit the bp. Even if it could, it seems brittle to have this implied rule that isn't stated in the protocol anywhere.

[ebluestein]

Just wanted to ping on the status of this, was this included in the Feb milestone?

[roblourens]

@weinand I think this repo needs some milestone cleanup, there is an open issue on November 2017

[weinand]

@ebluestein the original idea of the attributes "allThreadsStopped" and "threadId" was to express more or less what you are trying to do with the "threadCausedFocus": if all threads are stopped but one should be the "focused" thread then just set "allThreadsStopped" to true and set the "threadId" to the focused thread.

Would this work for you? If yes, I could just clarify the documentation.

/cc @isidorn

[ebluestein]

@weinand thanks for getting back to me! The allThreadsStopped can't quite do what I need, if my messages from the debugger backend are to be accurate.

The specific situation where I'm having a bit of trouble expressing what's going on is for our HHVM debugger backend (Hack/PHP debugger). It has a "stop-one, stop-all" policy, so if a web request thread hits a breakpoint, we pause all request threads. What happens though, is the other request threads stop totally asynchronously (the debugger gets opportunities to halt the thread only at certain points when they call back into the virtual machine, threads can execute for a bit in native code extensions, or running JIT'ed code before they execute an interrupt that can halt them).

Currently what I do is send a stoppedEvent with the threadID of the request that hit a breakpoint, and allThreadsStopped = false. The reason I do this is all threads are stoppING but they are not yet stopped, and I wanted to accurately report the truth about the state of the program from the debugger.

As the request threads finally call back into the virtual machine and get stopped, they send additional stop events, and finally allThreadsStopped = true when they've all halted.

This makes it pretty tough for the client UX to know which thread to select though - it's got all these stop events, which can actually arrive in any order.

I could send the first event with allThreadsStopped = true (which would be a bit of a lie), but since those threads haven't halted yet, the client cannot successfully ask for a stack trace or any variables from them, so this would break things too...

We've tried to work around this in our UX with various heuristics but they all felt hacky and sometimes pick the wrong thread. It seems like having the backend just say "pick this one" would be so much more bullet proof and makes our client UX logic dead simple

[ebluestein]

I don't know how much of a corner case this is. I can imagine other debuggers having similar issues where stopping threads can't be done atomically, not if you have only one thread (JavaScript) and probably not for a native target (C++) but I bet any language with a virtual machine that can call out into native extensions would have a similar problem

[weinand]

Thanks for explaining the details, now I understand better.

Since currently VS Code always focusses on StoppedEvents (which IMO is a good default behaviour), I wonder whether we can reverse the semantics of the new attribute and name it preserveFocusHint? So setting it to true prevents VS Code from automatically assigning the focus to this thread.

Here is the resulting definition:

/** Event message for 'stopped' event type.
    The event indicates that the execution of the debuggee has stopped due to some condition.
    This can be caused by a break point previously set, a stepping action has completed, by executing a debugger statement etc.
*/
export interface StoppedEvent extends Event {
  // event: 'stopped';
  body: {
    /** The reason for the event.
        For backward compatibility this string is shown in the UI if the 'description' attribute is missing (but it must not be translated).
        Values: 'step', 'breakpoint', 'exception', 'pause', 'entry', etc.
    */
    reason: string;
    /** The full reason for the event, e.g. 'Paused on exception'. This string is shown in the UI as is. */
    description?: string;
    /** The thread which was stopped. */
    threadId?: number;
    /** A value of true hints to the frontend that this event should not change the focus. */
    preserveFocusHint?: boolean;
    /** Additional information. E.g. if reason is 'exception', text contains the exception name. This string is shown in the UI. */
    text?: string;
    /** If allThreadsStopped is true, a debug adapter can announce that all threads have stopped.
     *  The client should use this information to enable that all threads can be expanded to access their stacktraces.
     *  If the attribute is missing or false, only the thread with the given threadId can be expanded.
     */
    allThreadsStopped?: boolean;
  };
}

[weinand]

@ebluestein the change is available in a branch: https://github.com/Microsoft/vscode-debugadapter-node/commit/ba5b70d4ea58fe56af226d56dd6dfeea7104eee7

Please let me know if you could support the reverse logic.

[ebluestein]

I think we could probably make that work if we really must, but the inverted logic seems more confusing to me in the protocol.

We have already implemented threadCausedFocus in Nuclide and a couple of our debugger back-ends (C++ and Hack/PHP), what we did to preserve the default behavior of selecting the thread (and for backwards compatibility) is to auto focus if threadCausedFocus is true or null, and not focus if it's === false

[weinand]

When introducing new boolean properties we always try to stick to JavaScript's "falsy" semantics: A missing property should have the same meaning as the value "false" of that property.

The current (implicit) semantic of the missing property is "thread caused focus". If we introduce a new property "threadCausedFocus" its value "false" would mean the opposite of the missing property. This would break the "falsy" semantics.

By reversing the meaning with a "preserveFocusHint" we can satisfy the "falsy" semantics.

If there are no strong objections I'm planning to merge https://github.com/Microsoft/vscode-debugadapter-node/commit/ba5b70d4ea58fe56af226d56dd6dfeea7104eee7 into master tomorrow.