http: fix undefined error in parser event #20029
Closed
Conversation
This file contains 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
The current check for socket.server[kIncomingMessage] does not account for the possibility of a socket.server that doesn't have that property defined. Fix it.
bnoordhuis
approved these changes
Apr 14, 2018
|
There's that |
lpinca
approved these changes
Apr 14, 2018
trivikr
approved these changes
Apr 14, 2018
jasnell
approved these changes
Apr 14, 2018
BridgeAR
approved these changes
Apr 15, 2018
|
About not adding a test: in a way this means it could break again at any point of time. So I personally would add a test even if we do not officially support this. |
BridgeAR
added
the
author ready
I'm fine with that. I don't want to add a test that forces us to maintain this behaviour if it's not sensible to do so in the future. (Even if it's just as simple as making someone hesitate to make an otherwise beneficial change.) There are already some difficult guarantees around what http does that impede certain changes that would be good for it. (The only way I know how to test this would also mean it would test a bunch of other behaviours that aren't documented and are just byproducts of how the internals are setup.) |
XadillaX
approved these changes
Apr 17, 2018
|
Landed in 95fafc0 |
apapirovski
added a commit
that referenced
this pull request
Apr 17, 2018
The current check for socket.server[kIncomingMessage] does not account for the possibility of a socket.server that doesn't have that property defined. Fix it. PR-URL: #20029 Fixes: #19231 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Khaidi Chu <i@2333.moe>
jasnell
pushed a commit
that referenced
this pull request
Apr 17, 2018
The current check for socket.server[kIncomingMessage] does not account for the possibility of a socket.server that doesn't have that property defined. Fix it. PR-URL: #20029 Fixes: #19231 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Khaidi Chu <i@2333.moe>
This was referenced Apr 19, 2018
trivikr
pushed a commit
to trivikr/node
that referenced
this pull request
Sep 15, 2018
The current check for socket.server[kIncomingMessage] does not account for the possibility of a socket.server that doesn't have that property defined. Fix it. PR-URL: nodejs#20029 Fixes: nodejs#19231 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Khaidi Chu <i@2333.moe>
2 tasks
MylesBorins
pushed a commit
that referenced
this pull request
Oct 31, 2018
The current check for socket.server[kIncomingMessage] does not account for the possibility of a socket.server that doesn't have that property defined. Fix it. Backport-PR-URL: #22880 PR-URL: #20029 Fixes: #19231 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Khaidi Chu <i@2333.moe>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current check for
socket.server[kIncomingMessage]does not account for the possibility of asocket.serverthat doesn't have that property defined. Fix it.No test as this isn't officially supported behaviour but the fix is simple so we might as well do it to not break the ecosystem.
(Would be nice to get this in v10.x if possible.)
Fixes: #19231
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes