http: lazy create IncomingMessage.headers #35281

ronag · 2020-09-21T10:26:14Z

When rawHeaders is enough don't create the headers object.

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-21T10:26:18Z

Review requested:

@nodejs/http
@nodejs/net

When rawHeaders is enough don't create the headers object.

lpinca · 2020-09-21T18:51:55Z

What are the advantages? I think working with raw headers is not common in user land and creating IncomingMessage.headers lazily might cause a performance loss.

ronag · 2020-09-21T19:29:19Z

I’m working a proxy library that can make use of this.

ronag · 2020-09-21T19:29:54Z

I suppose e.g. fastify could also make use of it? @mcollina

mcollina · 2020-09-21T19:41:55Z

Can we measure if there is a performance loss in adding this with express or fastify as they are right now? I think we might have a boost in postponing the parsing to where it's needed.

Overall I don't think this would change much our performance profile as in most cases those headers must be parsed. Proving rawHeaders enables the ecosystem to provide faster parsing options.

mscdex · 2020-09-21T20:23:12Z

The other thing to be aware of is that with this change, modifying .rawHeaders/.rawTrailers (before accessing .headers) can now affect the content of .headers.

ronag · 2020-09-21T20:56:39Z

The other thing to be aware of is that with this change, modifying .rawHeaders/.rawTrailers (before accessing .headers) can now affect the content of .headers.

Good point.

ronag · 2020-09-21T20:59:25Z

For context this is the scenario I'm trying to optimize for. Note how it only uses rawHeaders.

ronag · 2020-10-09T10:24:54Z

@nodejs/http

ronag · 2020-10-09T10:25:05Z

Anyone willing to help with benchmarking this?

jasnell · 2020-10-22T17:03:54Z

I'm not able to help with benchmarking this at the moment, but overall I'm fine with this basic idea

mcollina

lgtm

I do not see any improvement or regressions in any of the cases I tried. For the vast majority of the apps, this just shifts CPU work from one place to another.

mcollina · 2020-10-23T10:29:28Z

lib/_http_incoming.js

    }

-    for (let i = 0; i < n; i += 2) {
-      this._addHeaderLine(headers[i], headers[i + 1], dest);
+    if (dest) {


Suggested change

if (dest) {

if (dest !== null) {

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

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

ExE-Boss · 2020-12-17T12:38:44Z

@ronag That won’t fix the issue, as Object.assign and object spread only copies own enumerable properties, whereas this made IncomingMessage.headers into an accessor property on the prototype, and thus not an own property of the instance.

mcollina · 2020-12-17T18:02:02Z

This is relevant #36550 (comment)

This reverts commit b58725c. Fixes: #36550 PR-URL: #36553 Refs: #35281 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Refs: nodejs#35281 Refs: nodejs#36550 Co-authored-by: raisinten <[email protected]> PR-URL: nodejs#36601 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

Refs: #35281 Refs: #36550 Co-authored-by: raisinten <[email protected]> PR-URL: #36601 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

ronag · 2021-04-16T08:39:09Z

The headers are always calculated due to

node/lib/_http_server.js

Line 910 in d3162da

if (req.headers.expect !== undefined &&

😞

The IncomingMessage.headers property was made non-enumerable in PR nodejs#35281.

The IncomingMessage.headers property was made non-enumerable in PR #35281. PR-URL: #42095 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Harshitha K P <[email protected]>

The IncomingMessage.headers property was made non-enumerable in PR nodejs#35281. PR-URL: nodejs#42095 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Harshitha K P <[email protected]>

The IncomingMessage.headers property was made non-enumerable in PR #35281. PR-URL: #42095 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Harshitha K P <[email protected]>

The IncomingMessage.headers property was made non-enumerable in PR nodejs#35281. PR-URL: nodejs#42095 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Harshitha K P <[email protected]>

I tried the basic example in the README. It failed with a 500 Internal Server Error and the following JSON body: { "message": "Cannot read properties of undefined (reading 'x-forwarded-proto')", "name": "TypeError" } The output from the server process includes a stack trace. Sanitized a bit, it is: TypeError: Cannot read properties of undefined (reading 'x-forwarded-proto') at protocol (.../node_modules/paperplane/lib/parseUrl.js:18:14) at .../node_modules/ramda/src/converge.js:47:17 at _map (.../node_modules/ramda/src/internal/_map.js:6:19) at .../node_modules/ramda/src/converge.js:46:33 at f1 (.../node_modules/ramda/src/internal/_curry1.js:18:17) at .../node_modules/ramda/src/uncurryN.js:34:21 at .../node_modules/ramda/src/internal/_curryN.js:37:27 at .../node_modules/ramda/src/internal/_arity.js:10:19 at .../node_modules/ramda/src/internal/_pipe.js:3:14 at .../node_modules/ramda/src/internal/_arity.js:10:19 Yet the docker-based demo app works. That uses Node.js v12.22.12. When I tried that Node version on the basic example, it also worked. So, I used `nvm` to do a binary search on versions. I identified that Node.js v15.1.0 is the first failing version, and every version I tried that was newer (up to v21.2.0) also failed. Scouring the Node.js change log revealed that the http module of v15.1.0 started calculating req.headers lazily. There's a full discussion in nodejs/node#35281 [1]. Note the referenced issue that identifies the bug [2]. [1] nodejs/node#35281 [2] nodejs/node#36550 The workaround identified in this comment [3] is not to use the spread operator or `Object.assign` on the request object. "These objects are essentially uncloneable." [3] nodejs/node#36550 (comment) This is surprisingly tricky to do right. Various Ramda functions like `merge` (and I think `converge`) do this implicitly, as does funky's `assocWith`. So to work around this limitation, while preserving all behavior, I had to resort to (gasp) mutating procedures instead of pure functions. Not ideal, I know. I'm open to better ideas. So, maybe this isn't the best solution, but it least it gets the example working again on modern Node versions. If it never gets merged, at least it will be findable via the repo on GitHub. For reference, to test this, I used a fresh NPM project with only the paperplane dependency: mkdir paperplane cd paperplane npm init -y npm i -S paperplane In which I added an index.js file with these contents: const { compose } = require('ramda') const http = require('http') const { json, logger, methods, mount, routes } = require('paperplane') const cookies = req => ({ cookies: req.cookies || 'none?' }) const hello = req => ({ message: `Hello ${req.params.name}!` }) const app = routes({ '/' : methods({ GET: _ => ({ body: 'Hello anon.' }) }), '/cookies' : methods({ GET: compose(json, cookies) }), '/hello/:name': methods({ GET: compose(json, hello) }) }) http.createServer(mount({ app })).listen(3000, logger) And finally (with `fd` and `entr` and `httpie` installed), I started a file-watching auto-test process: fd -e js | entr -rcc bash -c "node index.js & http -v get http://localhost:3000/cookies Cookie:foo=bar"

ronag requested a review from mcollina September 21, 2020 10:26

ronag requested a review from a team as a code owner September 21, 2020 10:26

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

ronag changed the title ~~http: lazy create OutgoingMessage.headers~~ http: lazy create IncomingMessage.headers Sep 21, 2020

ronag force-pushed the http-lazy-headers branch 2 times, most recently from 52e6d16 to e839ab2 Compare September 21, 2020 10:27

http: lazy create IncomingMessage.headers

3417e0b

When rawHeaders is enough don't create the headers object.

ronag force-pushed the http-lazy-headers branch from e839ab2 to 3417e0b Compare September 21, 2020 10:28

This comment has been minimized.

Sign in to view

fixup

a9cc8f8

ronag force-pushed the http-lazy-headers branch from 9280724 to a9cc8f8 Compare September 21, 2020 12:07

fixup

ed40951

mcollina approved these changes Oct 23, 2020

View reviewed changes

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

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

This comment has been minimized.

Sign in to view

ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 23, 2020

This comment has been minimized.

Sign in to view

BethGriggs added the dont-land-on-v14.x label Dec 17, 2020

BethGriggs mentioned this pull request Dec 17, 2020

[v14.x] Revert "http: lazy create IncomingMessage.headers" #36553

Closed

2 tasks

ExE-Boss mentioned this pull request Dec 22, 2020

doc: document "http: lazy create IncomingMessage.headers" #36601

Merged

2 tasks

svkoskin mentioned this pull request Aug 27, 2021

Credentials provider: request headers undefined in authorize callback on new Nodes nextauthjs/next-auth#2612

Closed

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

ArnoldZokas added a commit to ArnoldZokas/node that referenced this pull request Feb 23, 2022

doc: document change to IncomingMessage.headers enumerability

19117ab

The IncomingMessage.headers property was made non-enumerable in PR nodejs#35281.

ArnoldZokas mentioned this pull request Feb 23, 2022

doc: document change to IncomingMessage.headers enumerability #42095

Merged

ArnoldZokas added a commit to ArnoldZokas/node that referenced this pull request Feb 23, 2022

doc: document change to IncomingMessage.headers enumerability

2acd93b

The IncomingMessage.headers property was made non-enumerable in PR nodejs#35281.

lionralfs mentioned this pull request Feb 26, 2022

cloneIncomingMessage does not handle headers field in Node.js v16 mswjs/interceptors#212

Closed

aortmannm mentioned this pull request Jul 2, 2022

[Bug]: Since the update to node.js 16 playing radio stations via script does not work iobroker-community-adapters/ioBroker.chromecast#124

Closed

efdali mentioned this pull request Oct 23, 2023

fix: forward-compatibility adaptation for request headers puzzle-js/puzzle-js#407

Merged

kyptin mentioned this pull request Dec 12, 2023

Fix mount for Node.js >= 15.1.0 articulate/paperplane#66

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: lazy create IncomingMessage.headers #35281

http: lazy create IncomingMessage.headers #35281

ronag commented Sep 21, 2020 •

edited

Loading

nodejs-github-bot commented Sep 21, 2020

This comment has been minimized.

lpinca commented Sep 21, 2020

ronag commented Sep 21, 2020

ronag commented Sep 21, 2020 •

edited

Loading

mcollina commented Sep 21, 2020

mscdex commented Sep 21, 2020

ronag commented Sep 21, 2020

ronag commented Sep 21, 2020

ronag commented Oct 9, 2020

ronag commented Oct 9, 2020

jasnell commented Oct 22, 2020

mcollina left a comment

mcollina Oct 23, 2020

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Oct 25, 2020

ExE-Boss commented Dec 17, 2020

mcollina commented Dec 17, 2020

ronag commented Apr 16, 2021

http: lazy create IncomingMessage.headers #35281

http: lazy create IncomingMessage.headers #35281

Conversation

ronag commented Sep 21, 2020 • edited Loading

Checklist

nodejs-github-bot commented Sep 21, 2020

This comment has been minimized.

lpinca commented Sep 21, 2020

ronag commented Sep 21, 2020

ronag commented Sep 21, 2020 • edited Loading

mcollina commented Sep 21, 2020

mscdex commented Sep 21, 2020

ronag commented Sep 21, 2020

ronag commented Sep 21, 2020

ronag commented Oct 9, 2020

ronag commented Oct 9, 2020

jasnell commented Oct 22, 2020

mcollina left a comment

Choose a reason for hiding this comment

mcollina Oct 23, 2020

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Oct 25, 2020

ExE-Boss commented Dec 17, 2020

mcollina commented Dec 17, 2020

ronag commented Apr 16, 2021

ronag commented Sep 21, 2020 •

edited

Loading

ronag commented Sep 21, 2020 •

edited

Loading