Feature Request: Option to configure highWaterMark of HTTP response

[trentmwillis]

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

We have a Node service that loads a bunch of data over an HTTP connection using Server-Sent Events. We noticed, when comparing to implementations in other languages, that the data we receive winds up being in many more "chunks" in Node which leads to some performance issues.

After some digging, it seems that the issue is that http.ServerResponse received when using http.request() uses the default highWaterMark value of 16kb inherited from the default ReadableStream class. This is much smaller than some of the messages we're processing which we believe is the cause of the perf issues since we must reconstruct the messages before parsing them fully.

Describe the solution you'd like

Ideally, an option to configure the highWaterMark size of a response when calling http.request.

It might be that there is a way to do what we need here, but I was not able to find it if so. So I'm open to feedback on that front.

Describe alternatives you've considered

Some way to override the default highWaterMark size of a stream (e.g., a CLI option), but that feels more expansive than needed.

Thanks in advance for your help/consideration 😃

[cjihrig]

It looks like passing readableHighWaterMark to http.request() already properly sets the highWaterMark on the stream's underlying socket. From there, the following patch propagates it to the IncomingMessage instance. Node's test suite passes for me with this change as well.

diff --git a/lib/_http_incoming.js b/lib/_http_incoming.js
index ad3699cc44..4f9c501377 100644
--- a/lib/_http_incoming.js
+++ b/lib/_http_incoming.js
@@ -37,7 +37,9 @@ function readStop(socket) {
 
 /* Abstract base class for ServerRequest and ClientResponse. */
 function IncomingMessage(socket) {
-  Stream.Readable.call(this);
+  let streamOptions;
+
+  if (socket) {
+    streamOptions = {
+      highWaterMark: socket.readableHighWaterMark
+    };
+  }
+
+  Stream.Readable.call(this, streamOptions);
 
   this._readableState.readingMore = true;

[trentmwillis]

@cjihrig thanks for looking into this! To clarify, does that mean the patch above will need to land for this to work as desired? The end result being you'd use it like so:

http.request('https://example.com', { readableHighWaterMark: 1000000 }, (response) => {
  console.log(response.readableHighWaterMark); // 1000000
});

[cjihrig]

So, thinking about this a bit more, I don't think we'd want to officially support passing readableHighWaterMark directly in http.request() and https.request() because it wouldn't work properly with agents that reuse existing sockets (AFAIK, changing the highWaterMark after stream construction is not supported). I think the better approach is to use the createConnection() option to http.request() or a custom agent to create sockets with the desired high water mark.

Unfortunately, I think we'd still need something like the patch above to set the highWaterMark of the IncomingMessage to match that of the socket, and that might end up as a semver-major change to the IncomingMessage class.

I've opened #30135 as a possible solution.

[trentmwillis]

Thanks for getting this implemented so quickly! I took a look at the PR and your solution makes sense and works for our use case 💯

[0807Jpatel]

Is there a way to do this on server side? http.createServer because hwm of response stream is larger than we would like it to be.