websockets / ws

Simple to use, blazing fast and thoroughly tested WebSocket client and server for Node.js
MIT License
21.33k stars 2.3k forks source link

Messages are dispatched while microtask queue is not empty #2216

Closed OrKoN closed 2 months ago

OrKoN commented 2 months ago

Is there an existing issue for this?

Description

I have previously reported https://github.com/websockets/ws/issues/2159 which was fixed but it looks like the solution might not have been complete.

With the latest version of ws the microtasks scheduled in the event callback are handled first but the microtasks created by those microtasks are not. This is different from the WebSocket client behavior in the browser.

Server code:

import { WebSocketServer } from 'ws';

const wss = new WebSocketServer({ port: 8080 });

wss.on('connection', function connection(ws) {
  ws.send('something1');
  ws.send('something2');
});

Client code:

import { WebSocket } from 'ws';

const ws = new WebSocket('ws://127.0.0.1:8080');

ws.addEventListener('message', function message(event) {
    console.log('received: %s', event.data);
    new Promise(resolve => {
      resolve()
    }).then(() => {
      console.log('microtask', event.data.toString())
      return Promise.resolve().then(() => {
        console.log('microtask-nested', event.data.toString());
      });
    })
});

ws version

8.16.0

Node.js Version

v20.10.0

System

System: OS: macOS 14.4.1 CPU: (10) arm64 Apple M1 Max Memory: 12.88 GB / 64.00 GB Shell: 5.9 - /bin/zsh

Expected result

received: something1
microtask something1
microtask-nested something1
received: something2
microtask something2
microtask-nested something2

Actual result

received: something1
microtask something1
received: something2
microtask-nested something1
microtask something2
microtask-nested something2

Attachments

No response

lpinca commented 2 months ago

That's because this is what happens under the hood.

queueMicrotask(function () {
  console.log('received: something1');
  Promise.resolve().then(() => {
    console.log('microtask something1');
    return Promise.resolve().then(() => {
      console.log('microtask-nested something1');
    });
  });
  queueMicrotask(function () {
    console.log('received: something2');
    Promise.resolve().then(() => {
      console.log('microtask something2');
      return Promise.resolve().then(() => {
        console.log('microtask-nested something2');
      });
    });
  });
});

We can get the expected behavior by resuming the parser when the microtask queue drains, for example by using setImmediate() instead of queueMicrotask(), but in that case we don't have one event per microtask but one event per loop iteration.

OrKoN commented 2 months ago

The WebSocket spec defines one event to be dispatched per task and not per microtask so setImmediate sounds appropriate. I am not sure if that degrades the performance though.

lpinca commented 2 months ago

Isn't a task in that context/spec a microtask?

I am not sure if that degrades the performance though.

It almost certainly does but there is the allowSynchronousEvents option now. Anyway setImmediate() might also have unintended side effects. I have to think about it.

OrKoN commented 2 months ago

It is a task but it is on a different queue, a microtask queue, in the html spec. The spec also says that a microtask queue is not a task queue so I am not sure. I think people also refer to tasks/global tasks as macro tasks as opposed to micro tasks.

OrKoN commented 2 months ago

https://html.spec.whatwg.org/multipage/webappapis.html#queue-a-task definitely puts a task on a task queue and not a microtask queue though.

lpinca commented 2 months ago

It looks like browsers do not dispatch one event per microtask so I guess I misunderstood the spec in https://github.com/websockets/ws/pull/2160.