socketio / socket.io-sticky

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

Http request with large request body is not working with socket sticky session #6

Closed kartikpandey2 closed 2 years ago

kartikpandey2 commented 2 years ago

Hi, Using socket sticky with nodejs cluster adapter. But for the post route with a large request body, it is holding on to request till it times out.

Steps to Reproduce

  1. Setup the repo
  2. Run server1.js(cluster without sticky session) file and make a post request on /test route with data in file body. The server will respond with a status ok.
  3. Now run server.js(cluster with sticky session) file and make a post request on /test route with data in file body. The server will not respond. But if we trim the request body it will work.
darrachequesne commented 2 years ago

Hi! Thanks for the working example, I could indeed reproduce the issue. Anything over 65 kb does indeed timeout :eyes:

kartikpandey2 commented 2 years ago

I tried to debug the issue. I find if we listen for connection event on HTTP server on the master process then only 1st chunk of data it's received. socket.on("data") is only called once.

const cluster = require("cluster");
const http = require("http");
const numCPUs = require("os").cpus().length;

if (cluster.isMaster) {
  const httpServer = http.createServer();

  const port = 3001;

  let recieved;

  httpServer.on("connection", (socket) => {
    socket.on("data", (buffer) => {
      const data = buffer.toString();
      recieved += data;

      console.log(recieved.length);
    });
  });

  httpServer.listen(port, () =>
    console.log('server started listening on port', port)
  );

  for (let i = 0; i < numCPUs; i++) {
    cluster.fork();
  }

  cluster.on("exit", (worker) => {
    console.log(`Worker ${worker.process.pid} died`);
    cluster.fork();
  });
} else {
  http.createServer();
}
kartikpandey2 commented 2 years ago

Update:- if I pass express app as an argument to both the master and worker server it starts working. But not working incase I don't pass express app

const cluster = require("cluster");
const http = require("http");
const numCPUs = require("os").cpus().length;
const { setupMaster } = require("@socket.io/sticky");
const { setupPrimary } = require("@socket.io/cluster-adapter");

const expressApp = require("./app");
const initSocket = require("./socket");

if (cluster.isMaster) {
  console.log(`Master ${process.pid} is running`);

  const httpServer = http.createServer(expressApp);

  // setup sticky sessions
  setupMaster(httpServer, {
    loadBalancingMethod: "least-connection",
  });

  // setup connections between the workers
  setupPrimary();

  // needed for packets containing buffers (you can ignore it if you only send plaintext objects)
  // Node.js < 16.0.0
  cluster.setupMaster({
    serialization: "advanced",
  });

  const port = 3001;
  httpServer.listen(port, () =>
    console.log(`server started listening on port ${port}`)
  );

  for (let i = 0; i < numCPUs; i++) {
    cluster.fork();
  }

  cluster.on("exit", (worker) => {
    console.log(`Worker ${worker.process.pid} died`);
    cluster.fork();
  });
} else {
  console.log(`Worker ${process.pid} started`);

  const httpServer = http.createServer(expressApp);

  initSocket(httpServer);
}
MatanYemini commented 2 years ago

Hi, will publish a fix for it soon.

@darrachequesne please consider the PR we will post, it is a very unpleasant behavior (I would call it - a fatal bug)

MatanYemini commented 2 years ago

https://github.com/socketio/socket.io-sticky/pull/9

darrachequesne commented 2 years ago

This should be fixed by https://github.com/socketio/socket.io-sticky/commit/a124d0beb5c1b78be5b75f10153859a9e4672862, included in version 1.0.2 :rocket: