Max requests per socket

[artur-ma]

Is your feature request related to a problem? Please describe.
We are scaling up pods in K8S on traffic spike, but since we are using keep alive, new pods are barley used since it takes too much time to rebalance the connections, as a result we have pods with high CPU usage, and pods with very low CPU usage.

Describe the solution you'd like
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Keep-Alive

HTTP standard support "max" parameter on "Keep-Alive" which defines that the connection can be used for up to "N" number of requests, then it will be closed.

The suggestion is to add this configuration on http.server that will return this parameter to the client, and also close the connection once it reached the maximum

Describe alternatives you've considered
This issue can be solved with side tools like service mesh (LinkerD or Istio) but they have their own issues and overhead.

We ended up doing it manually by creating some plugin for fastify that holds a WeakMap with a counter.

Some context: fastify/fastify#3260

The solution that we ended up with

  const counters = new WeakMap();
  ...
  let count = counters.get(socket)

  if (count === undefined) {
    count = 0;
  }

  counters.set(socket, ++count);

  if (count >= maxRequestsPerConnection) {
    reply.header('connection', 'close');
  }

[artur-ma]

I will try to contribute with a PR on this issue, but there are some questions

1. Should it close and the destroy the socket on reaching max? if so, how to avoid some race here and prevent some in-flight request to fail if there will be any, or just set the "connection: close" header without actually closing it.
2. I guess this(max parameter) should also should be supported by the client/agent?

[mcollina]

I think there are two places where we could implement a check and we might have to implement both.

The first is when we receive a new request, if we are over threshold we should be automatically respond with a 503.

The second is to add a check while preparing the keep-alive response header and decide not to keep alive the connection if the current connection is equal over threshold.

I recommend not to destroy the socket to avoid breaking any pipelined requests. We would likely need to maintain two counters (request received, request completed).

[mcollina]

I guess this(max parameter) should also should be supported by the client/agent?

I'm not sure it's needed as it will respect the connection:'close' header.

[artur-ma]

Another one, I plan to add the condition somwhere here
https://github.com/nodejs/node/blob/master/lib/_http_server.js#L906 in parserOnIncoming
but at this point the possible request body is not yet consumed from the socket(I assume), only the headers,
what will happen if I will call here:

      res.writeHead(503);
      res.end();

Will it understand that the socket buffer is still full and should be consumed, or it will just stuck and wait for the possible request body to be consumed by someone and only then the response will be sent

[mcollina]

That's the exact place where it should be put the "first" check I mentioned above.
calling res.end() respect pipelining, so it should be ok and doing what you expect it to do.

Write tests to verify this all.

[fatal10110]

as discussed, added a check on max requests, now is the hardest part
to understand how to run it locally, and how to run the tests

[fatal10110]

Looks like this

Should it close and the destroy the socket on reaching max

Already handled if "shouldKeepAlive" is set to false (it sets some _last=true flag)
https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js#L456

So overall it works (locally, for me)

But there is something odd, on using VSCode debugger, when reaching the

      res.writeHead(503);
      res.end();

(I dont know why I reach it only on debug mode, since as described above connection should be closed because of the _last flag, and it does for non-debug run)

server thorws

Uncaught Error [ERR_STREAM_WRITE_AFTER_END]: write after end

and dies...

[fatal10110]

Looks like some race condition, on calling destroy, we still get another request but we can not respond with 503 since it is already closed

My test client

const net = require('net');

let counter = 0;

const write = (client) => {

    counter++;
    client.write('GET / HTTP/1.1\r\n');
    client.write('Host: localhost:8080\r\n');
    client.write('Connection: keep-alive\r\n');
    client.write('\r\n\r\n')
}

const client = net.createConnection({ port: 8080 }, () => {
  
  write(client)
});

client.on('data', (data) => {
    console.log(data.toString());
    console.log('---------------');

  if (counter === 5) {
    client.end();
  } else {
    write(client)
  }
});

client.on('end', () => {
  console.log('disconnected from server');
});