Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose highWaterMark in http #39092

Closed
mmomtchev opened this issue Jun 19, 2021 · 7 comments
Closed

Expose highWaterMark in http #39092

mmomtchev opened this issue Jun 19, 2021 · 7 comments
Labels
feature request Issues that request new features to be added to Node.js. http Issues or PRs related to the http subsystem. stale

Comments

@mmomtchev
Copy link
Contributor

Is your feature request related to a problem? Please describe.

Recently I spent quite some time debugging some horrible network performance problem in a Node.js application. That application receives data from multiple HTTP streams (using Axios), most of them quite low bandwidth, with a few of them that can occasionally go over 20MB/s. I traced the problem down to Node.js not reading often enough from the socket, causing the kernel buffer to overflow, which in turn makes it set the TCP window to 0 with dramatic consequences to the throughput. The problem was that libuv was serving only 64k per event loop iteration. This means that an application trying to read at 20MB/s will have to do at least 320 event loop iterations per second - leaving it with a mere 3ms maximum allowed processing time per iteration - including GC and all.

There are two problems here: first, the occasional spike in processing time that should (and can be) absorbed by the kernel buffer. The default one on Linux, 262144 bytes, can absorb 12.5ms of data when receiving 20MB/s. This one can be adjusted by sysctl and does not concern Node.js.

The second problem is the average throughput. If the application cannot make an average of 320 loop iterations per second, that it's over - data keeps piling up and setting the TCP window to 0 is an awful way to control the flow.

Now, why the 64k limit.

libuv

First thing I noticed is that libuv reads in 64k buffers. This is merely a "suggestion" and can be adjusted by Node in EmitToJSStreamListener::OnStreamAlloc
There is a stale PR in libuv that discusses changing the "suggested size" on their side:
libuv/libuv#1279

When submitting read_cbs on Linux (why this logic is implemented in an OS-specific layer is beyond me) to the upper layers, libuv will do up to 32 reads (it is a protection to avoid starvation). This happens in the UNIX-specific uv__read. With 64k buffers this would have allowed me to achieve 20MB/s with only 10 event loop iterations per second. If only the HTTP client was not calling uv_read_stop at every teaspoon of data.

Node's HTTP client

Node's HTTP client will read the data and will stop when it reaches the highwatermark. The default highwatermark is at a meager 16k. It is 64k for files, but it is 16k for network sockets. Thankfully, Readable is capable of shoving the entire chunk of data up his buffer (Readable.prototype.push) before realizing it was too much. Older versions of Node (14) will get a first chunk of 16k and then a second one of 64k, newer ones will get 64k and will immediately realize they are full and emit a readStop.

highWaterMark

So, how do we set the highwatermark of http.request? I would like to open an issue in axios, but I am afraid that if I present the solution I have found, they will tell me that they are not interested in supporting undocumented Node.js internals:

options.createConnection = (opts) => {
  opts.highWaterMark = 1024 * 1024;
  const socket = new require('net').Socket(opts);
  if (opts.timeout) {
    socket.setTimeout(opts.timeout);
  }
  return socket.connect({
    host: opts.host,
    port: opts.port
  });
}
http.request(options, cb);

Describe the solution you'd like

options.highWaterMark = 1024 * 1024;
http.request(options, cb);

Also, probably consider raising the default value of 16k.

@mscdex
Copy link
Contributor

mscdex commented Jun 19, 2021

undocumented Node.js internals

Which part are you referring to? That example solution looks completely fine to me.

Also I believe your example solution for existing versions of node can be improved a bit FWIW with something like:

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

function request(opts, cb) {
  return http.request({
    ...opts,
    createConnection: (options) => net.connect({
      ...options,
      highWaterMark: 1024 * 1024,
      // or with node v14.0.0+ to affect just the readable side
      //readableHighWaterMark: 1024 * 1024,
    })
  }, cb);
}

which will automatically set the timeout for you if there is one and will not mutate the caller's options object.

@mscdex mscdex added feature request Issues that request new features to be added to Node.js. http Issues or PRs related to the http subsystem. labels Jun 19, 2021
@mmomtchev
Copy link
Contributor Author

@mscdex, yes indeed the need to set the timeout was the worst part, but even in this form, I will still have a hard time selling it to axios

@mmomtchev
Copy link
Contributor Author

@mscdex, maybe, ideally, the buffer should start at 16Kb, than double its size for every iteration it is full up to a limit, maybe 256Kb or even more, and also maybe half it every time it is less than half full
But I wonder if it is worth the debug/development/maintenance to implement such a complex mechanism or just leave it to the end user to raise the value should he need it

@targos
Copy link
Member

targos commented Jun 19, 2021

Axios supports passing a custom http.Agent. Is there no way to set the highWaterMark with that?

@mmomtchev
Copy link
Contributor Author

@targos, Yes, in fact there is - I forgot that

@github-actions
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Mar 29, 2022
@github-actions
Copy link
Contributor

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. http Issues or PRs related to the http subsystem. stale
Projects
Development

No branches or pull requests

3 participants