net: set default highwatermark at socket creation time#48882
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot commented Jul 22, 2023
Review requested:
|
nodejs-github-bot added needs-ci 
lib/net.js Outdated
| if(typeofoptions.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.
ShogunPanda commented Jul 22, 2023
Can you also please fix linter issues? |
lukianoforce-pushed the fix/watermark-at-socket-creation branch from a6dc57b to d6e7851CompareShogunPanda commented Jul 22, 2023
LGTM now. |
mcollina commented Jul 24, 2023
Tests are failing |
lukianoforce-pushed the fix/watermark-at-socket-creation branch from d6e7851 to 624f697Comparelukiano commented Jul 24, 2023
I've amended the tests. |
lukianoforce-pushed the fix/watermark-at-socket-creation branch from 624f697 to b0572e0Comparelukiano commented Jul 24, 2023
I've also made the procedure more evident in the documentation. |
lukianoforce-pushed the fix/watermark-at-socket-creation branch from b0572e0 to 9f859b9Comparelukiano commented Jul 24, 2023
Fixed some linting issues. |
lpinca added the semver-major 
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.
lukianoforce-pushed the fix/watermark-at-socket-creation branch from 9f859b9 to 18696dfCompare
github-actionsbot removed the request-ci nodejs-github-bot commented Jul 28, 2023
github-actionsbot removed the request-ci nodejs-github-bot commented Aug 10, 2023
aduh95 commented Sep 29, 2023
This needs a rebase to fix the git conflicts. |
lukiano commented Sep 29, 2023
Thanks. Rebased |
github-actionsbot removed the request-ci nodejs-github-bot commented Sep 30, 2023
github-actionsbot removed the request-ci nodejs-github-bot commented Oct 30, 2023
nodejs-github-bot commented Nov 11, 2023
nodejs-github-bot commented Nov 20, 2023
nodejs-github-bot commented Nov 20, 2023
nodejs-github-bot commented Nov 21, 2023
github-actionsbot removed the request-ci nodejs-github-bot commented Dec 26, 2023
aduh95 commented Feb 23, 2025
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. |
7 participants
This PR is to make a recent code changes backwards compatible with previous releases of Node.JS, as depicted in #47405 (comment)