FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http: lazy create IncomingMessage.headers by ronag · Pull Request #35281 · nodejs/node · GitHub

/ node Public

http: lazy create IncomingMessage.headers - #35281

Closed
ronag wants to merge 3 commits into
nodejs:masterfrom
nxtedition:http-lazy-headers
Closed

http: lazy create IncomingMessage.headers#35281
ronag wants to merge 3 commits into
nodejs:masterfrom
nxtedition:http-lazy-headers

Conversation

ronag commented Sep 21, 2020
edited
Loading

Copy link
Copy Markdown
Member

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

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

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

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
When rawHeaders is enough don't create the headers object.

This comment has been minimized.

lpinca commented Sep 21, 2020

Copy link
Copy Markdown
Member

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 commented Sep 21, 2020

Copy link
Copy Markdown
Member Author

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

ronag commented Sep 21, 2020
edited
Loading

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member

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 commented Sep 21, 2020

Copy link
Copy Markdown
Contributor

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 commented Sep 21, 2020

Copy link
Copy Markdown
Member Author

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 commented Sep 21, 2020

Copy link
Copy Markdown
Member Author

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

ronag commented Oct 9, 2020

Copy link
Copy Markdown
Member Author

@nodejs/http

ronag commented Oct 9, 2020

Copy link
Copy Markdown
Member Author

Anyone willing to help with benchmarking this?

jasnell commented Oct 22, 2020

Copy link
Copy Markdown
Member

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

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread lib/_http_incoming.js

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
if (dest) {
if (dest !== null) {

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.

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

This comment has been minimized.

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Contributor

@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.

Copy link
Copy Markdown
Member

This is relevant #36550 (comment)

BethGriggs added a commit that referenced this pull request Dec 17, 2020
This reverts commit b58725c.

Fixes: #36550

PR-URL: #36553
Refs: #35281
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Trott pushed a commit to ExE-Boss/node that referenced this pull request Dec 25, 2020
Refs: nodejs#35281
Refs: nodejs#36550

Co-authored-by: raisinten <raisinten@gmail.com>

PR-URL: nodejs#36601
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
Refs: #35281
Refs: #36550

Co-authored-by: raisinten <raisinten@gmail.com>

PR-URL: #36601
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

ronag commented Apr 16, 2021

Copy link
Copy Markdown
Member Author

The headers are always calculated due to

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

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
The IncomingMessage.headers property was made non-enumerable
in PR nodejs#35281.
nodejs-github-bot pushed a commit that referenced this pull request Feb 23, 2022
The IncomingMessage.headers property was made non-enumerable
in PR #35281.

PR-URL: #42095
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Harshitha K P <harshitha014@gmail.com>
sxa pushed a commit to sxa/node that referenced this pull request Mar 7, 2022
The IncomingMessage.headers property was made non-enumerable
in PR nodejs#35281.

PR-URL: nodejs#42095
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Harshitha K P <harshitha014@gmail.com>
danielleadams pushed a commit to danielleadams/node that referenced this pull request Apr 21, 2022
The IncomingMessage.headers property was made non-enumerable
in PR nodejs#35281.

PR-URL: nodejs#42095
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Harshitha K P <harshitha014@gmail.com>
danielleadams pushed a commit that referenced this pull request Apr 24, 2022
The IncomingMessage.headers property was made non-enumerable
in PR #35281.

PR-URL: #42095
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Harshitha K P <harshitha014@gmail.com>
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
The IncomingMessage.headers property was made non-enumerable
in PR nodejs#35281.

PR-URL: nodejs#42095
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Harshitha K P <harshitha014@gmail.com>
kyptin added a commit to kyptin/paperplane that referenced this pull request Dec 12, 2023
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"
kyptin added a commit to kyptin/paperplane that referenced this pull request Dec 12, 2023
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"
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. http Issues or PRs related to the http subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants


Back | FazBrowse Home | New Git URL