socketio / socket.io-sticky

A simple and performant way to use Socket.IO within a cluster.
https://socket.io
MIT License
43 stars 13 forks source link

leastActive worker is undefined when socket io worker servers are built with an express server #10

Closed wvhulle closed 1 year ago

wvhulle commented 1 year ago

There is a problem with the function computeWorkerId when express is used to construct the socket io server. When all the socket worker servers have been closed, the following error appears:

/home/wvhulle/Documents/socket.io-fiddle/node_modules/@socket.io/sticky/index.js:58
        return leastActiveWorker.id;
                                 ^

TypeError: Cannot read properties of undefined (reading 'id')
    at computeWorkerId (/home/wvhulle/Documents/socket.io-fiddle/node_modules/@socket.io/sticky/index.js:58:34)
    at Socket.<anonymous> (/home/wvhulle/Documents/socket.io-fiddle/node_modules/@socket.io/sticky/index.js:80:18)
    at Socket.emit (node:events:525:35)
    at addChunk (node:internal/streams/readable:324:12)
    at readableAddChunk (node:internal/streams/readable:297:9)
    at Readable.push (node:internal/streams/readable:234:10)
    at TCP.onStreamRead (node:internal/stream_base_commons:190:23)

It seems like the function cannot deal with the case where there are no workers. When I remove express, the problem disappears.

The bug can be seen in https://github.com/wvhulle/socket.io-fiddle/tree/cluster-adapter, after running the client and the server, then waiting until the worker threads quit.

wvhulle commented 1 year ago

Adding app.close() in the workers does not seem to help.

But adding this in the primary

  cluster.on("exit", (worker) => {
    console.log(`Worker ${worker.process.pid} died`);
    if (Object.values(cluster.workers).length === 0) {
      httpServer.close()
    }
  });

solves the problem, but maybe this library should also have something opposite to setupMaster? Like closeMaster? Or otherwise be able to deal with data still coming in the primary server but no workers available.

wvhulle commented 1 year ago
  cluster.on("exit", (worker) => {
    console.log(`Worker ${worker.process.pid} died`);
    if (Object.values(cluster.workers).length === 0) {
      httpServer.prependListener("connection", (socket) => { socket.destroy() })
    }
  });

Also seems to work.

darrachequesne commented 1 year ago

@wvhulle I could indeed reproduce, thanks. We should definitely handle the case where there are no available worker.

wvhulle commented 1 year ago

Should I open a merge request with one of my fixes?

darrachequesne commented 1 year ago

That would be awesome 👍

darrachequesne commented 1 year ago

For future readers:

This should be fixed by https://github.com/socketio/socket.io-sticky/commit/e8b4203d18fc9601e05af3457baba49fafdb15f0, included in version 1.0.3. Please reopen if needed!

wvhulle commented 1 year ago

Thanks a lot!