socketio / socket.io-protocol

Socket.IO Protocol specification
https://socket.io
507 stars 62 forks source link

waitFor latency causes the test-suite to miss some packets #32

Open Totodore opened 1 year ago

Totodore commented 1 year ago

Currently, for each message check this function is called to wait for a message. However it causes a lot of failed checks because the latency of this function is quite high (because of the new event listener and the promise).

function waitFor(socket, eventType) {
    return new Promise((resolve) => {
        socket.addEventListener(
            eventType,
            (event) => {
                resolve(event);
            },
            { once: true }
        );
    });
}

It became really problematic with buffered messages. Recently I have added a flush mechanism on socketioxide to avoid flushing the websocket for each socket.io packet. Also because the engine.io connect packet is sent immediately after the websocket initialisation it may not be handled because of the same issue.

Here is an example where the waitFor function makes the test break and a hacky solution. However this happens almost everywhere. This issue can be reproductible even with the official test server with the following command to overload a bit the nodejs event loop :

seq 10 | parallel -j 10 'npm test -- -f "should connect then disconnect from a custom namespace"; echo '

it("should connect then disconnect from a custom namespace", async () => {
    const socket = await initSocketIOConnection();

    await waitFor(socket, "message"); // ping

    // Create a waiting promise *before* sending the trigger packet "40/custom" makes the trick
    const handshakeHandler = waitForPackets(socket, 2);
    socket.send("40/custom");

    // await waitFor(socket, "message"); // Socket.IO handshake
    // await waitFor(socket, "message"); // auth packet
    await handshakeHandler;

    socket.send("41/custom");
    socket.send('42["message","message to main namespace"]');

    const { data } = await waitFor(socket, "message");
    expect(data).to.eql('42["message-back","message to main namespace"]');
});

I didn't find any good solution to propose a PR. If there is no solution to this issue. I may consider rewriting the test suite in another language to avoid event loop issues. Maybe @darrachequesne you have an idea ?

Totodore commented 1 year ago

I found a solution with the nodejs stream adapter from the ws lib and the async iterator provided by node :

const socket = createWebSocketStream(new WebSocket(
    `${WS_URL}/socket.io/?EIO=4&transport=websocket`
  ), { objectMode: true, readableObjectMode: true });

socket.binaryType = "arraybuffer";

// The async iterator does not spawn a Promise + an event listener at each call
// Also the stream handles bufferred packets

await socket.iterator().next() // Engine.IO handshake

socket._write("40");

await socket.iterator().next(); // Socket.IO handshake
await socket.iterator().next(); // "auth" packet

return socket;

I don't know yet if I make a thin adapter for the ws socket so there is nothing much changed in all the test suites (socket.io + engine.io v3 & v4) or if I rewrite all the test suites with the async iterator way.

I'm open to suggestions :)