Process debug adapter messages in separate tasks; see #33822, #79196

[dgozman]

Common pattern when dealing with bidirectional channel is:

channel.onSomeEvent(event => handleEvent(event));
...
const response = await channel.sendCommand(params);

If we consider the other side of the cannel, it often sends events in between request and response:

handleSomeCommand(args): {
  ...
  this.sendSomeEvent(params);
  ...
  return result;
}

This generates the following sequence of messages: [someEvent, someCommandResponse], which is handled by the channel in the order of someEvent, someCommandResponse.

However, someEvent is then dispatched synchronously (see onSomeEvent above), while someCommandResponse is handled through a promise (see await sendCommand above). More general, various event handlers and command response handlers contain arbitrary promise chains of different lengths before being handled. This turns commands/events which did arrive in the right order during a single javascript task - into handlers which run in absolutely different order due to different chains of javascript microtasks.

To guarantee the order of processing between various events and command responses, we can artificially insert javascript task boundaries between them. This ensures that any javascript microtasks related to the first event/response handling will finish before the next event/response is processed, no matter how long the promise chain was, thanks to the javascript tasks spec.

[dgozman]

@isidorn Could you please take a look? This is another part of #81196. I did include a test which demonstrates the behavior.

For manual testing, evaluate setTimeout(() => console.log(2), 0); 1 and observe 1, then 2 instead of 2, then 1.

Unfortunately, I did not find a primitive from async which works here, because they:

• either drop messages on the floor (like Throttler), and we cannot afford that;
• or operate on promises/microtasks (like Sequencer), which does not solve the issue because there is no hard task boundary introduced.

[isidorn]

Love the tests.

[isidorn]

I do not understand why you need this.queue! (I mean the explamation mark at the end)
this.queue is never null and you properly initiazlie it to an empty array. So you should be always safe to access this.queue.

As for the ! after the shift, I would prefer to just check if the message is there by if !(message) {return}. Just looks safer to me.

[isidorn]

@dgozman I like that all the changes are kept on the debugAdapter level which is great.

Couple of things:

1. Why don't you use Queue and between each item put a promise await timeout(0). Not sure if that simplifies the whole thing?
2. We are wrapping up our milestone and I am taking the day off tomorrow, thus I will prefer that we merge this in next milestone (in 10 days) so we get time to test it properly before shipping to stable. Thus assigning to October.

fyi @weinand on the changes. Best described in @dgozman initial comment

[dgozman]

Thank you, @isidorn. Pushing this to October sounds good to me, this PR is riskier than average :) I will try to use the Queue as you suggested, will see whether it will be easier.

[weinand]

Let's discuss this when I'm back from vacation.

[isidorn]

Assigning to @weinand so he does his review. I did an initial review.
If we decide to go with this approach I think we shuold merge earlier in the milestone so we have time for testing.

[connor4312]

I address the comments, tests still pass 🙂

[isidorn]

Thanks a lot! This looks great to me and now is the perfect time to merge this (start of milestone).
@connor4312 did you verify that microsoft/vscode-js-debug#122 gets fixed with this?

I @weinand does not oppose I will merge this in tomorrow.

[connor4312]

It doesn't unfortunately--I didn't root cause it earlier, but it looks like the main problem is actually that the terminated output is written on the parent debug session, whereas the test output comes from the child. Disregarding this bug, we actually do preserve order of output messages for a single debug session, but not across all debug sessions--and I'm not sure that we should: if one session is e.g. deadlocked and not sending variables, should we hold up the output of all other sessions?

I was holding off pushing more on that for microsoft/debug-adapter-protocol#85, a DAP feature we need to resolve a separate problem. If we go with the implementation I suggested in that issue, then that will also solve microsoft/vscode-js-debug#122 by removing the async pathway.

[isidorn]

Ok, thanks for clariyfing.
For now let's merge this in, since i like these changes anyway.
And for follow up work let's see how that DAP feature discussions evolves.