http: lazy create IncomingMessage.headers by ronag · Pull Request #35281 · nodejs/node
ronag
changed the title
http: lazy create OutgoingMessage.headers
http: lazy create IncomingMessage.headers
ronag
added
the
author ready
label
Oct 23, 2020BethGriggs added a commit that referenced this pull request
Dec 17, 2020This 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>
BethGriggs added a commit that referenced this pull request
Dec 17, 2020This 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, 2020Refs: 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, 2021ArnoldZokas added a commit to ArnoldZokas/node that referenced this pull request
Feb 23, 2022The IncomingMessage.headers property was made non-enumerable in PR nodejs#35281.
ArnoldZokas added a commit to ArnoldZokas/node that referenced this pull request
Feb 23, 2022The IncomingMessage.headers property was made non-enumerable in PR nodejs#35281.
nodejs-github-bot pushed a commit that referenced this pull request
Feb 23, 2022The 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, 2022The 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, 2022The 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, 2022The 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>
danielleadams pushed a commit that referenced this pull request
Apr 24, 2022The 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>
danielleadams pushed a commit that referenced this pull request
Apr 24, 2022The 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, 2022The 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, 2023I 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, 2023I 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