Make order of ConfigurationDoneRequest and LaunchRequest deterministic

[weinand]

Currently the order of ConfigurationDoneRequest and LaunchRequest is not deterministic, that means it is possible that a ConfigurationDoneRequest comes after the LaunchRequest. This is problematic because only a ConfigurationDoneRequest indicates that all breakpoints have been registered and that a LaunchRequest is able to hit all those breakpoints. If the ConfigurationDoneRequest comes after the LaunchRequest some breakpoints might be missed.

There are two scenarios to consider:

1. An InitializedEvent is sent early (after the InitializeRequest has been processed but before the LaunchRequest). This is used in mono-debug and mock-debug where breakpoints can be registered before launching the debuggee.
2. An InitializedEvent is sent late (while the LaunchRequest is being processed). This is used in node-debug where node.js has to be launched first before breakpoints can be registered).

From the first case it looks like the frontend could always wait with calling LaunchRequest until ConfigurationDoneRequest has been ended. But this would not work with the second case.

/cc @isidorn

[weinand]

@andrewcrawley here is the corresponding issue from 2016!

I've moved it to the February milestone.

[andrewcrawley]

vsdbg is another example of an adapter that sends the "initialized" event during processing of the "launch" request, and I believe MIEngine + OpenDebugAD7 also exhibits this behavior.

If an adapter advertises that it supports "configurationDone", but begins executing the debuggee before receiving that request, it seems pretty clear to me that this is a bug in the adapter. Running the debugee obviously requires complete configuration information, so if configuration is still ongoing, the debuggee should not be running. I would expect that in the case that the debug adapter receives "launch" before "configurationDone", it would:

• Validate the "launch" parameters as much as possible and return failure if (e.g.) the debuggee does not exist.
• Launch the process in a "suspended" state, or cache the information for later
• Once the "configurationDone" request is received, either resume the process (if it was launched and suspended), or launch it directly using the cached info

CC: @gregg-miskelly @caslan @richardstanton

[gregg-miskelly]

I am not following how the first scenario above could work reliably. If I am reading the following bolded section of the protocol documentation for InitializedEvent correctly --

A debug adapter is expected to send this event when it is ready to accept configuration requests (but not before the InitializeRequest has finished)

Then to me that means that a debug adapter isn't allowed to send the InitializedEvent before it sends InitializeResponse.

If I am reading things correctly -- there would be a race in scenario 1 since just because the debug adapter sent the InitializedEvent right away doesn't mean the client will actually get that event in time to know that it needs to delay sending launch until all the breakpoints and other configuration options have been sent.

[weinand]

@gregg-miskelly thanks for adding the excerpt from the InitializedEvent spec.

I had hoped that the implementation of mock-debug would actually stick to the spec, but it does not. It failed the "(but not before the InitializeRequest has finished)".

Moving the line that emits the InitializedEvent to the end of initializeRequest (after the this.sendResponse(response)) made mock-debug fail (which confirmed my speculation that mock-debug was just lucky being able to run with VS Code).

Trying to delay the launchRequest until the response of the configurationDone request has been received would only work for the "early" case but obviously not for the "late" case because there the InitializedEvent is emitted in the launchRequest.

Making this approach work for the "late" case, would require that the frontend receives the information whether it is dealing with the "early" or the "late" case. This would require another capability... which I don't like.

@andrewcrawley this means that I've (finally!) arrived at the same conclusion as you did already a while ago: the problem cannot be (easily) solved on the frontend side but must be fixed in mock-debug.

So today I tried various approaches to delay the launch request until the configurationDone request has been received. Most of them were really ugly (and they were actually the motivation for my quest for trying to fix it in the frontend).

But in the end I found a wait/notify-based fix that is small and simple: https://github.com/Microsoft/vscode-mock-debug/blob/9d378d6aa00982c565b9c19323a7882538a6063a/src/mockDebug.ts#L111-L134

@andrewcrawley Please try whether the latest version of mock-debug works for you now.

[andrewcrawley]

No, it still doesn't work in the VS host. We wait for a response to the "launch" request before we proceed with initialization, so with your fix, the launch request blocks for 1000ms, then continues anyway, still before the "configurationDone" request.

Would it work to send the response, then do your wait before starting the runtime?

Here's the log from our side:

 1> 02/06/2018 13:27:38.448: [DebugAdapter] --> C (initialize-1): {"command":"initialize","arguments":{"clientID":"visualstudio","adapterID":"mock","locale":"en-US","linesStartAt1":true,"columnsStartAt1":true,"pathFormat":"path","supportsVariableType":true,"supportsRunInTerminalRequest":true,"supportsHandshakeRequest":true},"seq":1,"type":"request"}
 1> 02/06/2018 13:27:58.921: [DebugAdapter] <--   R (initialize-1): {"seq":1,"type":"response","request_seq":1,"command":"initialize","success":true,"body":{"supportsConfigurationDoneRequest":true,"supportsEvaluateForHovers":true,"supportsStepBack":true}}
 1> 02/06/2018 13:27:58.926: [DebugAdapter] <--   E (initialized): {"seq":2,"type":"event","event":"initialized"}
 1> 02/06/2018 13:27:58.988: [DebugAdapter] --> C (launch-2): {"command":"launch","arguments":{"noDebug":false,"type":"mock","request":"launch","program":"c:\\Users\\andrewcr\\source\\repos\\MockDebugTest\\MockDebug.txt"},"seq":2,"type":"request"}
 1> 02/06/2018 13:27:59.992: [DebugAdapter] <--   R (launch-2): {"seq":3,"type":"response","request_seq":2,"command":"launch","success":true}
 1> 02/06/2018 13:27:59.995: [DebugAdapter] <--   E (stopped): {"seq":4,"type":"event","event":"stopped","body":{"reason":"exception","threadId":1}}
 1> 02/06/2018 13:28:00.084: [DebugAdapter] --> C (threads-3): {"command":"threads","arguments":{},"seq":3,"type":"request"}
 1> 02/06/2018 13:28:00.085: [DebugAdapter] --> C (setBreakpoints-4): {"command":"setBreakpoints","arguments":{"source":{"path":"c:\\Users\\andrewcr\\source\\repos\\MockDebugTest\\MockDebug.txt","checksums":[]},"breakpoints":[{"line":4,"column":1}],"lines":[4]},"seq":4,"type":"request"}
 1> 02/06/2018 13:28:00.105: [DebugAdapter] <--   R (threads-3): {"seq":5,"type":"response","request_seq":3,"command":"threads","success":true,"body":{"threads":[{"id":1,"name":"thread 1"}]}}
 1> 02/06/2018 13:28:00.134: [DebugAdapter] --> C (setExceptionBreakpoints-5): {"command":"setExceptionBreakpoints","arguments":{"filters":[]},"seq":5,"type":"request"}
 1> 02/06/2018 13:28:00.196: [DebugAdapter] --> C (stackTrace-6): {"command":"stackTrace","arguments":{"threadId":1,"startFrame":0,"levels":1000},"seq":6,"type":"request"}
 1> 02/06/2018 13:28:00.219: [DebugAdapter] <--   R (setBreakpoints-4): {"seq":6,"type":"response","request_seq":4,"command":"setBreakpoints","success":true,"body":{"breakpoints":[{"verified":true,"line":4,"id":1}]}}
 1> 02/06/2018 13:28:00.225: [DebugAdapter] <--   E (breakpoint): {"seq":7,"type":"event","event":"breakpoint","body":{"reason":"changed","breakpoint":{"verified":true,"id":1}}}
 1> 02/06/2018 13:28:00.227: [DebugAdapter] <--   R (setExceptionBreakpoints-5): {"seq":8,"type":"response","request_seq":5,"command":"setExceptionBreakpoints","success":true}
 1> 02/06/2018 13:28:00.228: [DebugAdapter] --> C (configurationDone-7): {"command":"configurationDone","arguments":{},"seq":7,"type":"request"}
 1> 02/06/2018 13:28:00.229: [DebugAdapter] <--   R (stackTrace-6): {"seq":9,"type":"response","request_seq":6,"command":"stackTrace","success":true,"body":{"stackFrames":[{"id":0,"source":{"name":"MockDebug.txt","path":"c:\\Users\\andrewcr\\source\\repos\\MockDebugTest\\MockDebug.txt","sourceReference":0,"adapterData":"mock-adapter-data"},"line":6,"column":0,"name":"Throw(0)"},{"id":1,"source":{"name":"MockDebug.txt","path":"c:\\Users\\andrewcr\\source\\repos\\MockDebugTest\\MockDebug.txt","sourceReference":0,"adapterData":"mock-adapter-data"},"line":6,"column":0,"name":"an(1)"},{"id":2,"source":{"name":"MockDebug.txt","path":"c:\\Users\\andrewcr\\source\\repos\\MockDebugTest\\MockDebug.txt","sourceReference":0,"adapterData":"mock-adapter-data"},"line":6,"column":0,"name":"exception(2)"},{"id":3,"source":{"name":"MockDebug.txt","path":"c:\\Users\\andrewcr\\source\\repos\\MockDebugTest\\MockDebug.txt","sourceReference":0,"adapterData":"mock-adapter-data"},"line":6,"column":0,"name":"here.(3)"}],"totalFrames":4}}
 1> 02/06/2018 13:28:00.231: [DebugAdapter] <--   R (configurationDone-7): {"seq":10,"type":"response","request_seq":7,"command":"configurationDone","success":true}
 1> 02/06/2018 13:28:03.922: [DebugAdapter] --> C (continue-8): {"command":"continue","arguments":{"threadId":1},"seq":8,"type":"request"}
 1> 02/06/2018 13:28:03.939: [DebugAdapter] <--   R (continue-8): {"seq":11,"type":"response","request_seq":8,"command":"continue","success":true}
 1> 02/06/2018 13:28:03.951: [DebugAdapter] <--   E (terminated): {"seq":12,"type":"event","event":"terminated"}
 1> 02/06/2018 13:28:03.957: [DebugAdapter] --> C (disconnect-9): {"command":"disconnect","arguments":{},"seq":9,"type":"request"}
 1> 02/06/2018 13:28:03.959: [DebugAdapter] <--   R (disconnect-9): {"seq":13,"type":"response","request_seq":9,"command":"disconnect","success":true}

[weinand]

Delaying the "initialisation" to occur after the "launch" request has send its response, sounds wrong.
And I see nothing in the spec that asks for this interpretation.

Does the VS host always delay the processing of the "initialisation" after the "launch" response is received?
Did you verify any other DA besides mock-debug, e.g. node-debug, node-debug2 or python-debug?

"launch" means "launch the debuggee". It does not mean "remember the launch config for some later point in time".

If the launching fails, the error is reported as the "launch" response. Returning a launch error as a "configurationDoneResponse" will confuse frontends as it is not specified that a launch error can be returned as such.
So the answer to your question:

Would it work to send the response, then do your wait before starting the runtime?

is "no".

In addition, if breakpoints arrive as part of the "initialisation" after the program has beed launched, there is the danger that breakpoints in the startup sequence are not hit because execution is already beyond that.

We have introduced the "initialiseEvent" to avoid the problem that the DA has to remember all config information until it can actually be used. With the "initialiseEvent" the DA can request the information at a time when it is able to use it.

If this "initialise" sequence is now delayed by the frontend until the "launch" request has sent its response, this means for some DAs that they now have to remember all launch information until the "configurationDone" request is called. This basically negates the reason why the "initialiseEvent" got introduced in the first place.

[andrewcrawley]

To recap - from our earlier conversation, it sounds like the logic in VS Code is basically:

• Send initialize request
  • When initialized event is received:
    • Send configuration sequence: setBreakpoints, setFunctionBreakpoints, etc, followed by configurationDone
  • When initialize response is received:
    • Send launch request

In the case of an adapter that tries to do "early" initialization by sending the initialize request during processing of the initialize response (such as the "mock debug" adapter), this results in a race condition:

• Send initialize request
• Receive initialized event
• Receive initialize response
• Send setBreakpoints request (in response to initialized event)
• Send launch request (in response to initialize response)
• Receive setBreakpoints response, send setFunctionBreakpoints request

In this case, the debuggee is now running, but hasn't received the setFunctionBreakpoints request yet, so it may miss an early function breakpoint or exception. Your fix to the "mock debug" adapter works around this by making the "launch" request wait for up to 1000ms if the configurationDone request has not been received yet, but this is not robust - for example, if the host is slow for some reason, the adapter will quickly continue running without proper configuration anyway.

As you mentioned in our earlier conversation, it may be possible for the host to guess (based on the ordering of events and responses it receives) when it's safe to send the "launch" request, but this would break down if an adapter returns an unexpected message ordering. This may be acceptable if you want to specify that certain orderings are just "wrong", but this would need to be made clear in the protocol spec - right now it's very open to interpretation.

The solution I have proposed (don't do anything that requires valid configuration until the configurationDone request is received) is basically how VS solves this problem:

• VS asks you to launch the program
• You launch the program (generally in a suspended state)
• You send a "program created" event to VS
• VS sends you all the breakpoints, exception settings, etc
• VS informs you that it's finished processing your "program created" event
• You begin executing debugee code

This maps very closely to the "late" initialization sequence.

I surveyed various popular debug adapters and found the following implementations:

• Send initialized event during processing of launch request (aka "late" initialization): node2, go, php, python, powershell, ruby, vsdbg
• Send initialized event BEFORE initialize response (aka "early" initialization): mock-debug
• Send initialized event AFTER initialize response, but BEFORE launch request (aka ???): mono

Are you aware of other adapters that use "early" initialization or another weird sequence of events?

[weinand]

No, the logic (defined in the spec and embodied in VS Code) is this:

• Send initialize request
• Wait for initialize response
• Send launch request
• Wait for launch response
• Debuggee has started (or not in case of errors).

Concurrent to this the DA may send an initialise event to trigger the "initialise" sequence of VS Code.
The earliest point in time to send this event is after the initialise response has been sent (mock-debug got this wrong, which is now fixed and mono-debug got this right).
There is no latest point in time to send this event (because VS Code will always respond to this event).
However, in order to finish the initialisation sequence in time for a successful launch, the initialise event must be sent before the actual launch takes place which means it must be sent before the launch response is sent back to VS Code.

In contrast VS uses the "don't do anything that requires valid configuration until the "configurationDone" request is received" approach. This changes the semantics of the "launch" request significantly (which would result in a - may be breaking - change of the DAP spec) because "launch" doesn't do anything but collecting parameters. "configurationDone" becomes the real work horse now (another change of the DAP spec).

BTW, what is your sequence if a DA does not request "configurationDone" (which is the default)?

I'm not saying that the current VS Code approach is any better than your approach. I just don't understand what effect your approach has on any existing DAs out there.

What I know is that all existing DAs (even the broken mock-debug) work with the logic implemented by VS Code.
I do not know that the same statement holds for VS: at least mock-debug (even the fixed) does not work in VS.
But even if we fix mock-debug once more, there is no proof that other DAs will work without modification under VS.

Your comments about my mock-debug fix:

(Please ignore the 1000 ms timeout for a moment).
The DA has issued an initialise event and has requested (via a capability) to receive a configurationDone event.
When handling the launch request the DA knows for sure whether the (requested) configurationDone event has arrived or not. If not, it starts to wait for it before it actually launches the debuggee. Since it does not want to launch the debuggee without having received all breakpoints, it waits indefinitely. If this is too long, we can use a timeout to either continue the launch without the breakpoints, or we can make the launch request fail. I did the former (which is a bad choice as you've pointed out). The correct approach is to make the launch request fail.

[andrewcrawley]

I think we're in agreement on the logic, my formatting is just a bit different. If you disagree, can you clarify what I got wrong?

The VS host requires an initialized event. I suppose the event isn't technically necessary for a debug adapter that doesn't support any sort of breakpoints, exceptions, etc, but we haven't run across one that doesn't send initialized yet. Our (current) sequence looks like:

1. Send initialize request and wait for response
2. Send launch request and wait for response
3. Wait for initialized event, then send breakpoints, etc, then send configurationDone.

If an adapter doesn't support configurationDone, the VS host follows the same pattern, with the only difference being that we send setExceptionBreakpoints in step 3 even if there are none (and obviously we don't send configurationDone). This has worked with every adapter we've tested except for mock-debug (and assumedly mono-debug, but I haven't tried that)

I can investigate making the VS host not block on the launch request before proceeding with initialization.

[weinand]

Yes, we are in agreement with the logic and you've clearly stated where VS and VS Code differ:

I can investigate making the VS host not block on the launch request before proceeding with initialization.

So today the exact VS sequence is probably more this:

1. Send initialize request and wait for response
2. Start to listen for initialized event, but do not process initialized event until launch response received.
3. Send launch request and wait for response
4. If not yet received wait for initialized event, then send breakpoints, etc, then send configurationDone.

Important consequence: Initialisation always starts after launch response received (which means that every(!) DA has to move the actually launching of the debuggee into the configurationDone request). Those that don't do this might receive breakpoint etc. too late.

And the VS Code sequence is:

1. Send initialize request and wait for response
2. Start to listen for initialized event, and upon receipt immediately send breakpoints, etc, then send configurationDone.
3. Send launch request and wait for response

Important consequence: Initialisation starts immediately and can occur before or after the launch response arrives. This makes it possible to actually launch the debuggee in the launch request (after waiting for the configurationDone request to arrive).

So if you could get the VS host to work when not blocking on the launch request, we could be very sure that the VS and VS Code behave identically and that all DAs would work fine in the VS host.

[weinand]

I'm resolving this issue by specifying two constraints:

1. the Initialize event must be emitted after sending the Initialize response
2. the Launch/Attach response must be sent after sending the ConfigurationDone response

The following sequence diagrams show the exact sequence for the "early" and the "late" case:

Early case:

Late case: