net: set default highwatermark at socket creation time#48882
net: set default highwatermark at socket creation time#48882lukiano wants to merge 1 commit intonodejs:mainfrom
Conversation
|
Review requested:
|
lib/net.js
Outdated
| if (typeof options.highWaterMark !== 'undefined') { | ||
| validateNumber( | ||
| options.highWaterMark, 'options.highWaterMark', | ||
| 0 |
There was a problem hiding this comment.
I'm throwing an error if options.highWaterMark is less than zero. Alternatively, we can fallback to a default value of undefined.
There was a problem hiding this comment.
I'm okay with being more strict and forbid negative values.
lib/net.js
Outdated
| this.keepAlive = Boolean(options.keepAlive); | ||
| this.keepAliveInitialDelay = ~~(options.keepAliveInitialDelay / 1000); | ||
| this.highWaterMark = options.highWaterMark ?? getDefaultHighWaterMark(); | ||
| this.highWaterMark = options.highWaterMark ?? undefined; |
There was a problem hiding this comment.
Theoretically this.highWaterMark = options.highWaterMark; should suffice if we are happy with falsy values.
There was a problem hiding this comment.
Yes, let's accept falsy values.
|
Can you also please fix linter issues? |
a6dc57b to
d6e7851
Compare
|
LGTM now. |
|
Tests are failing |
d6e7851 to
624f697
Compare
|
I've amended the tests. |
624f697 to
b0572e0
Compare
|
I've also made the procedure more evident in the documentation. |
b0572e0 to
9f859b9
Compare
|
Fixed some linting issues. |
doc/api/http.md
Outdated
| `readableHighWaterMark` and `writableHighWaterMark`. This affects | ||
| `highWaterMark` property of both `IncomingMessage` and `ServerResponse`. | ||
| **Default:** See [`stream.getDefaultHighWaterMark()`][]. | ||
| **Default:** `undefined`. Sockets will use the default watermark value at the time the request arrives. See [`stream.getDefaultHighWaterMark()`][]. |
There was a problem hiding this comment.
I would keep it consistent and add a line break after 80 characters.
There was a problem hiding this comment.
The lint job fails for this reason.
9f859b9 to
18696df
Compare
|
This needs a rebase to fix the git conflicts. |
|
Thanks. Rebased |
|
This needs another rebase unfortunately |
|
This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open. |
|
Closing this because it has stalled. Feel free to reopen if this issue/PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions. |
This PR is to make a recent code changes backwards compatible with previous releases of Node.JS, as depicted in #47405 (comment)