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.

Sign up for GitHub

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

Jump to bottom

Make it possible to set Response/Request highWaterMark on http.Server #46606

Closed
ronag opened this issue Feb 10, 2023 · 6 comments · Fixed by #47405
Closed

Make it possible to set Response/Request highWaterMark on http.Server #46606

ronag opened this issue Feb 10, 2023 · 6 comments · Fixed by #47405
Labels
feature request Issues that request new features to be added to Node.js. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. http Issues or PRs related to the http subsystem.

Comments

@ronag
Copy link
Member

ronag commented Feb 10, 2023 •
edited
Loading

It would be helpful to have an option on http.createServer that sets the highWaterMark on all sockets created. The default highWaterMark is IMHO kind of low for most modern server hardware...
@ronag ronag added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. http Issues or PRs related to the http subsystem. feature request Issues that request new features to be added to Node.js. labels Feb 10, 2023
@ronag
Copy link
Member Author

ronag commented Feb 10, 2023

@nodejs/performance

@ronag ronag changed the title Make it possible to set ServerResponse/Request high water mark on Server Make it possible to set ServerResponse/Request highWaterMark on http.Server Feb 10, 2023
@ronag ronag changed the title Make it possible to set ServerResponse/Request highWaterMark on http.Server Make it possible to set Response/Request highWaterMark on http.Server Feb 10, 2023
@mcollina
Copy link
Member

I think we can safely increase it. Maybe bring it to 32KB both ways?

@ronag
Copy link
Member Author

ronag commented Feb 10, 2023

I think we can safely increase it. Maybe bring it to 32KB both ways?

#46608 does takes us some of the way but I think it's varies from case to case how high one can go, e.g a large amazon instance vs a raspberry pi.

@benjamingr
Copy link
Member

Was about to repeat what Matteo said - can we just increase it?

If we can't do it safely across all envs - can we just increase it based on some parameter of the system?

@ronag
Copy link
Member Author

ronag commented Feb 10, 2023

Some push back in #46608

@HinataKah0
Copy link
Contributor

HinataKah0 commented Mar 22, 2023 •
edited
Loading

Hi,
I am interested to work on the changes.

What I have tried,

Option 1:

  1. Store the new opt in storeHTTPOptions
  2. Set it when calling IncomingMessage
  3. Set it when calling ServerResponse, similar to setting kUniqueHeaders

Draft Commit

Option 2:

Set both (readable|writable)HighWaterMark when creating a new Socket.

Draft Commit

I'll open a PR. If this is no longer needed, please close it. 😄
Thanks!

@HinataKah0 HinataKah0 mentioned this issue Apr 4, 2023
Merged
HinataKah0 added a commit to HinataKah0/node that referenced this issue Apr 21, 2023
@HinataKah0
http: add highWaterMark opt in http.createServer
25a6e25 
Add highWaterMark option when creating a new HTTP server.
This option will override the default (readable|writable)
highWaterMark values on sockets created.

Fixes: nodejs#46606
nodejs-github-bot pushed a commit that referenced this issue Apr 24, 2023
@HinataKah0
http: add highWaterMark opt in http.createServer
2fbe124 
Add highWaterMark option when creating a new HTTP server.
This option will override the default (readable|writable)
highWaterMark values on sockets created.

Fixes: #46606
PR-URL: #47405
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
yjl9903 pushed a commit to yjl9903/node that referenced this issue Apr 28, 2023
@HinataKah0 @yjl9903
http: add highWaterMark opt in http.createServer
1558294 
Add highWaterMark option when creating a new HTTP server.
This option will override the default (readable|writable)
highWaterMark values on sockets created.

Fixes: nodejs#46606
PR-URL: nodejs#47405
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
yjl9903 pushed a commit to yjl9903/node that referenced this issue Apr 28, 2023
@HinataKah0 @yjl9903
http: add highWaterMark opt in http.createServer
dba7d82 
Add highWaterMark option when creating a new HTTP server.
This option will override the default (readable|writable)
highWaterMark values on sockets created.

Fixes: nodejs#46606
PR-URL: nodejs#47405
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
yjl9903 pushed a commit to yjl9903/node that referenced this issue Apr 29, 2023
@HinataKah0 @yjl9903
http: add highWaterMark opt in http.createServer
5a3c4de 
Add highWaterMark option when creating a new HTTP server.
This option will override the default (readable|writable)
highWaterMark values on sockets created.

Fixes: nodejs#46606
PR-URL: nodejs#47405
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
targos pushed a commit that referenced this issue May 2, 2023
@HinataKah0 @targos
http: add highWaterMark opt in http.createServer
4fa7739 
Add highWaterMark option when creating a new HTTP server.
This option will override the default (readable|writable)
highWaterMark values on sockets created.

Fixes: #46606
PR-URL: #47405
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
danielleadams pushed a commit that referenced this issue Jul 6, 2023
@HinataKah0 @danielleadams
http: add highWaterMark opt in http.createServer
46ee19c 
Add highWaterMark option when creating a new HTTP server.
This option will override the default (readable|writable)
highWaterMark values on sockets created.

Fixes: #46606
PR-URL: #47405
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this issue Jul 6, 2023
@HinataKah0 @MoLow
http: add highWaterMark opt in http.createServer
7cef6aa 
Add highWaterMark option when creating a new HTTP server.
This option will override the default (readable|writable)
highWaterMark values on sockets created.

Fixes: nodejs#46606
PR-URL: nodejs#47405
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature request Issues that request new features to be added to Node.js. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

http: add highWaterMark opt in http.createServer HinataKah0/node
4 participants
@mcollina @benjamingr @ronag @HinataKah0