http: add highWaterMark opt in http.createServer

[HinataKah0]

Add highWaterMark option when creating a new HTTP server.
This option will override the default (readable|writable)
highWaterMark values on sockets created.

Fixes: #46606

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/net

[HinataKah0]

Added doc, test no option case, undefined check.
Please take another look 😄

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/51113/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/51117/

[HinataKah0]

Since some time has passed, I'll make it as ready to review 😄

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/51397/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/51435/

[HinataKah0]

@ronag We have passed CI
Because it's a bit painful to pass the CI with these flaky tests 😅
Shall we merge it before we run into merge conflicts and need to re-run the CI again?

[lukiano]

I was looking at this change as part of the 18.17.0 release and noticed that the highWaterMark is cached when the HTTP server is created.

There's a (pretty dodgy) scenario on the following code

const http = require('http');
const stream = require('stream');

const server = new http.Server((request, response) => {
  console.log(`request highWatermark ${request.readableHighWaterMark}`);
  console.log(`response highWatermark ${response.writableHighWaterMark}`);
  response.setHeader('Connection', 'close');
  response.end();
});

server.listen(8080);
// Change the default highWaterMark after 5 seconds.
setTimeout(() => stream.setDefaultHighWaterMark(false, 32768), 5000);

that, on Node 20.0.0 gives an output of

request highWatermark 16384
response highWatermark 16384
request highWatermark 16384
response highWatermark 16384
request highWatermark 32768
response highWatermark 32768

these are 3 requests, where the third is after the 5 seconds timeout in the logic.
In Node 20.1.0, gives an output of

request highWatermark 16384
response highWatermark 16384
request highWatermark 16384
response highWatermark 16384
request highWatermark 16384
response highWatermark 16384

as the 16384 value is cached when the server is created and then passed to new requests.

In node 18, this change and the introduction of stream.setDefaultHighWaterMark() are done on the same release 18.17.0.

In Node 20 this change lands in 20.1.0 whereas stream.setDefaultHighWaterMark() was introduced in Node 19. Would that mean, then, that this change is technically a breaking change in Node 20?

[ShogunPanda]

I'm not sure I understand your question: can you please rephrase it?

[lukiano]

I think that in net.js, in the Server constructor:

if (typeof options.highWaterMark !== 'undefined') {
  validateNumber(
    options.highWaterMark, 'options.highWaterMark',
  );
  if (options.highWaterMark < 0) {
    options.highWaterMark = getDefaultHighWaterMark();
  }
}
this.highWaterMark = options.highWaterMark ?? getDefaultHighWaterMark();

should have been

if (typeof options.highWaterMark !== 'undefined') {
  validateNumber(
    options.highWaterMark, 'options.highWaterMark',
  );
  if (options.highWaterMark < 0) {
    // either throw an error or set to undefined
  }
}
this.highWaterMark = options.highWaterMark; // do not set any default value if undefined

Essentially, leaving this.highWaterMark undefined if nothing is set in the constructor options.

When a new socket is created, it will call getDefaultHighWaterMark() because self.highWaterMark is undefined.

It fully maintains backward compatibility by calling getDefaultHighWaterMark() at Socket construction time instead of at Server construction time.

[ShogunPanda]

Oh, I see. Yes, it seems to make sense to me. Are you willing to send a PR?