GeezerVison

Copy link

Copy Markdown

Member

bricss reacted with eyes emoji

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

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

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

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

mcollina approved these changes Oct 23, 2020

View reviewed changes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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]>

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 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"

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"