Repository navigation
Feature Request: Option to configure highWaterMark of HTTP response #30107
Description
Activity
- changed the title
[-]Option to configure `highWaterMark` of HTTP `response`[/-][+]Feature Request: Option to configure `highWaterMark` of HTTP `response`[/+]on Oct 24, 2019 It looks like passing
readableHighWaterMarktohttp.request()already properly sets the highWaterMark on the stream's underlying socket. From there, the following patch propagates it to theIncomingMessageinstance. 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;
Reacted by Gireesh Punathil, ZYSzys and Leo Dutra@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 });
So, thinking about this a bit more, I don't think we'd want to officially support passing
readableHighWaterMarkdirectly inhttp.request()andhttps.request()because it wouldn't work properly with agents that reuse existing sockets (AFAIK, changing thehighWaterMarkafter stream construction is not supported). I think the better approach is to use thecreateConnection()option tohttp.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
highWaterMarkof theIncomingMessageto match that of the socket, and that might end up as a semver-major change to theIncomingMessageclass.I've opened #30135 as a possible solution.
- added a commit that references this issue
on Oct 30, 2019 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 💯
Reacted by Colin Ihrig- added a commit that references this issue
on Nov 5, 2019 - added a commit that references this issue
on Jan 12, 2020 - added a commit that references this issue
on Feb 6, 2020 Is there a way to do this on server side?
http.createServerbecause hwm ofresponse streamis larger than we would like it to be.Reacted by KodjoSuprem, jdunk and Adam Žingor
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.ServerResponsereceived when usinghttp.request()uses the defaulthighWaterMarkvalue of16kbinherited from the defaultReadableStreamclass. 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
highWaterMarksize of aresponsewhen callinghttp.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
highWaterMarksize of a stream (e.g., a CLI option), but that feels more expansive than needed.Thanks in advance for your help/consideration 😃