http: allow passing array of key/val into writeHead by ronag · Pull Request #35274 · nodejs/node · GitHub

Enables an optimization when the user already has the headers
in an array form, e.g. when proxying.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Review requested:

@nodejs/http
@nodejs/net

Enables an optimization when the user already has the headers in an array form, e.g. when proxying.

This needs docs changes, right?

@addaleax Added docs

Enables an optimization when the user already has the headers in an array form, e.g. when proxying.

Why would proxying imply the user gets the headers in array form?

undici provides them in array form

lgtm

@benjamingr Here we would benefit from being able to pass the array directly without first converting it into an object.

CI: https://ci.nodejs.org/job/node-test-pull-request/33182/

I still don't understand this feature request/PR. Is this is a specific format that afaik only unidici uses?

Wouldn't it make more sense to support raw headers as a string?

Is this is a specific format that afaik only unidici uses?

At the moment yes... since Node doesn't have this feature yet no one has optimized for it as far as I'm aware...

Wouldn't it make more sense to support raw headers as a string?

Sure... But since we already do arrays I don't see any advantage. I guess one does not exclude the other.

Sure... But since we already do arrays I don't see any advantage. I guess one does not exclude the other.

I'm confused by this. This is an intermediary format that only one library uses. Passing a string of headers (concatenated) is something I've seen in many servers/languages.

Would the optimisation be impossible without going through the third (array) format?

I'm confused by this.

Due to the way our http parser works the most efficient way to get headers from node is through arrays (i.e. rawHeaders & rawTrailers). If we pass it as string it becomes inefficient to process the headers.

For best performance in core and user land should use flat arrays for headers. Strings don't allow efficient introspection and objects are expensive to create.

@mcollina maybe you are able to formulate this better?

For best performance in core and user land should use flat arrays for headers. Strings don't allow efficient introspection and objects are expensive to create.

Do you have any benchmark for this? How much faster is creating an array instead of a plain object? Or is this related to how the HTTP parser works?

Do you have any benchmark for this? How much faster is creating an array instead of a plain object? Or is this related to how the HTTP parser works?

The parser outputs an Array.

Ok, but in this case the parser is not used no? Isn't OutgoingMessage writing directly to the socket?

Edit: Ah I get it now, it's for proxying. I guess the idea is to get the headers as received by the parser and directly forward them.

@benjamingr comment added

CI: https://ci.nodejs.org/job/node-test-pull-request/33307/

CI: https://ci.nodejs.org/job/node-test-pull-request/33488/

CI: https://ci.nodejs.org/job/node-test-pull-request/33490/

Commit Queue failed

- Loading data for nodejs/node/pull/35274
✔  Done loading data for nodejs/node/pull/35274
----------------------------------- PR info ------------------------------------
Title      http: allow passing array of key/val into writeHead (#35274)
Author     Robert Nagy  (@ronag)
Branch     ronag:write-head-array -> nodejs:master
Labels     author ready, http, semver-minor
Commits    6
 - http: allow passing array of key/val into writeHead
 - fixup: docs
 - fixup
 - fixup
 - fixup
 - fixup
Committers 1
 - Robert Nagy 
PR-URL: https://github.com/nodejs/node/pull/35274
Reviewed-By: Matteo Collina 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Rich Trott 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/35274
Reviewed-By: Matteo Collina 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Rich Trott 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - fixup
   ⚠  - fixup
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2020-10-09T12:42:51Z: https://ci.nodejs.org/job/node-test-pull-request/33490/
- Querying data for job/node-test-pull-request/33490/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Sun, 20 Sep 2020 07:33:30 GMT
   ✔  Approvals: 3
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/35274#pullrequestreview-492926190
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/35274#pullrequestreview-494531574
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/35274#pullrequestreview-494545046
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

\nCommit Queue action: https://github.com/nodejs/node/actions/runs/298285943

Landed in d70c0ed

Enables an optimization when the user already has the headers in an array form, e.g. when proxying. PR-URL: #35274 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Notable changes: crypto: * update certdata to NSS 3.56 (Shelley Vohr) #35546 doc: * add aduh95 to collaborators (Antoine du Hamel) #35542 fs: * (SEMVER-MINOR) add rm method (Ian Sutherland) #35494 http: * (SEMVER-MINOR) allow passing array of key/val into writeHead (Robert Nagy) #35274 src: * (SEMVER-MINOR) expose v8::Isolate setup callbacks (Shelley Vohr) #35512 PR-URL: TODO

Notable changes: crypto: * update certdata to NSS 3.56 (Shelley Vohr) #35546 doc: * add aduh95 to collaborators (Antoine du Hamel) #35542 fs: * (SEMVER-MINOR) add rm method (Ian Sutherland) #35494 http: * (SEMVER-MINOR) allow passing array of key/val into writeHead (Robert Nagy) #35274 src: * (SEMVER-MINOR) expose v8::Isolate setup callbacks (Shelley Vohr) #35512 PR-URL: #35648

Enables an optimization when the user already has the headers in an array form, e.g. when proxying. PR-URL: nodejs#35274 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Notable changes: crypto: * update certdata to NSS 3.56 (Shelley Vohr) nodejs/node#35546 doc: * add aduh95 to collaborators (Antoine du Hamel) nodejs/node#35542 fs: * (SEMVER-MINOR) add rm method (Ian Sutherland) nodejs/node#35494 http: * (SEMVER-MINOR) allow passing array of key/val into writeHead (Robert Nagy) nodejs/node#35274 src: * (SEMVER-MINOR) expose v8::Isolate setup callbacks (Shelley Vohr) nodejs/node#35512 PR-URL: nodejs/node#35648

ronag requested a review from mcollina September 20, 2020 07:33

ronag requested a review from a team as a code owner September 20, 2020 07:33

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Sep 20, 2020

http: allow passing array of key/val into writeHead

7c3bcf5

Enables an optimization when the user already has the headers in an array form, e.g. when proxying.

ronag force-pushed the write-head-array branch from c308f0f to 7c3bcf5 Compare September 20, 2020 07:33

mcollina reviewed Sep 20, 2020

View reviewed changes

Comment thread lib/_http_server.js

fixup: docs

9c94f5b

addaleax reviewed Sep 20, 2020

View reviewed changes

Comment thread doc/api/http.md

fixup

f8f3e05

ronag force-pushed the write-head-array branch from 23d54d9 to f8f3e05 Compare September 21, 2020 07:27

mcollina approved these changes Sep 21, 2020

View reviewed changes

ronag added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Sep 21, 2020

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2020

targos reviewed Sep 22, 2020

View reviewed changes

Comment thread lib/_http_server.js

Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 22, 2020

ronag requested a review from addaleax September 30, 2020 18:41

ronag added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Sep 30, 2020

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2020

fixup

4097ae4

ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 9, 2020

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 9, 2020

fixup

cfe68c7

ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 9, 2020

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 9, 2020

ronag added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 9, 2020

github-actions Bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 9, 2020

github-actions Bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Oct 9, 2020

ronag closed this Oct 10, 2020

MylesBorins mentioned this pull request Oct 14, 2020

v14.14.0 proposal #35648

Merged

targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021

pimterry mentioned this pull request Dec 6, 2022

Support sending raw headers websockets/ws#2103

Closed

1 task

http: allow passing array of key/val into writeHead by ronag · Pull Request #35274 · nodejs/node · GitHub

Uh oh!

Uh oh!

http: allow passing array of key/val into writeHead - #35274

http: allow passing array of key/val into writeHead#35274ronag wants to merge 6 commits intonodejs:masternodejs/node:masterfrom nxtedition:write-head-arraynxtedition/node:write-head-arrayCopy head branch name to clipboard

Conversation

ronag commented Sep 20, 2020

Checklist

Uh oh!

nodejs-github-bot commented Sep 20, 2020

Uh oh!

Uh oh!

addaleax commented Sep 20, 2020

Uh oh!

ronag commented Sep 20, 2020

Uh oh!

Uh oh!

benjamingr commented Sep 21, 2020

Uh oh!

ronag commented Sep 21, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Sep 21, 2020

Uh oh!

nodejs-github-bot commented Sep 21, 2020

Uh oh!

benjamingr commented Sep 22, 2020

Uh oh!

ronag commented Sep 22, 2020

Uh oh!

benjamingr commented Sep 22, 2020

Uh oh!

ronag commented Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

lpinca commented Sep 22, 2020

Uh oh!

ronag commented Sep 22, 2020

Uh oh!

lpinca commented Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Sep 30, 2020

Uh oh!

nodejs-github-bot commented Sep 30, 2020

Uh oh!

nodejs-github-bot commented Oct 9, 2020

Uh oh!

nodejs-github-bot commented Oct 9, 2020

Uh oh!

github-actions Bot commented Oct 9, 2020

Uh oh!

ronag commented Oct 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

http: allow passing array of key/val into writeHead#35274
ronag wants to merge 6 commits into
nodejs:masterfrom
nxtedition:write-head-array

ronag commented Sep 22, 2020 •
edited
Loading

lpinca commented Sep 22, 2020 •
edited
Loading