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

ronag · 2020-09-20T07:33:30Z

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

nodejs-github-bot · 2020-09-20T07:33:34Z

Review requested:

@nodejs/http
@nodejs/net

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

lib/_http_server.js

addaleax · 2020-09-20T14:25:10Z

This needs docs changes, right?

ronag · 2020-09-20T20:24:18Z

@addaleax Added docs

doc/api/http.md

benjamingr · 2020-09-21T16:51:33Z

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?

ronag · 2020-09-21T17:12:16Z

undici provides them in array form

mcollina

lgtm

ronag · 2020-09-21T20:58:09Z

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

nodejs-github-bot · 2020-09-21T21:19:08Z

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

benjamingr · 2020-09-22T07:54:14Z

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?

ronag · 2020-09-22T08:34:56Z

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.

benjamingr · 2020-09-22T08:37:52Z

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?

ronag · 2020-09-22T08:45:00Z

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?

lib/_http_server.js

lpinca · 2020-09-22T18:44:52Z

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?

ronag · 2020-09-22T19:03:33Z

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.

lpinca · 2020-09-22T19:10:44Z

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.

ronag · 2020-09-30T18:42:28Z

@benjamingr comment added

nodejs-github-bot · 2020-09-30T18:50:07Z

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

nodejs-github-bot · 2020-10-09T10:25:22Z

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

nodejs-github-bot · 2020-10-09T12:42:51Z

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

github-actions · 2020-10-09T21:54:29Z

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

ronag · 2020-10-10T08:12:23Z

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

lib/_http_server.js Show resolved Hide resolved

fixup: docs

9c94f5b

addaleax reviewed Sep 20, 2020

View reviewed changes

doc/api/http.md Show resolved Hide resolved

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 pending requests for changes, 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

lib/_http_server.js Show resolved Hide resolved

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 pending requests for changes, 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ronag commented Sep 20, 2020

nodejs-github-bot commented Sep 20, 2020

addaleax commented Sep 20, 2020

ronag commented Sep 20, 2020

benjamingr commented Sep 21, 2020

ronag commented Sep 21, 2020

mcollina left a comment

ronag commented Sep 21, 2020

nodejs-github-bot commented Sep 21, 2020

benjamingr commented Sep 22, 2020

ronag commented Sep 22, 2020

benjamingr commented Sep 22, 2020

ronag commented Sep 22, 2020 •

edited

Loading

lpinca commented Sep 22, 2020

ronag commented Sep 22, 2020

lpinca commented Sep 22, 2020 •

edited

Loading

ronag commented Sep 30, 2020

nodejs-github-bot commented Sep 30, 2020

nodejs-github-bot commented Oct 9, 2020

nodejs-github-bot commented Oct 9, 2020

github-actions bot commented Oct 9, 2020

ronag commented Oct 10, 2020

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

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

Conversation

ronag commented Sep 20, 2020

Checklist

nodejs-github-bot commented Sep 20, 2020

addaleax commented Sep 20, 2020

ronag commented Sep 20, 2020

benjamingr commented Sep 21, 2020

ronag commented Sep 21, 2020

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Sep 21, 2020

nodejs-github-bot commented Sep 21, 2020

benjamingr commented Sep 22, 2020

ronag commented Sep 22, 2020

benjamingr commented Sep 22, 2020

ronag commented Sep 22, 2020 • edited Loading

lpinca commented Sep 22, 2020

ronag commented Sep 22, 2020

lpinca commented Sep 22, 2020 • edited Loading

ronag commented Sep 30, 2020

nodejs-github-bot commented Sep 30, 2020

nodejs-github-bot commented Oct 9, 2020

nodejs-github-bot commented Oct 9, 2020

github-actions bot commented Oct 9, 2020

ronag commented Oct 10, 2020

ronag commented Sep 22, 2020 •

edited

Loading

lpinca commented Sep 22, 2020 •

edited

Loading