http: destroy the socket on parse error by lpinca · Pull Request #24757 · nodejs/node

lpinca · 2018-11-30T18:31:37Z

Destroy the socket if the 'clientError' event is emitted and there is
no listener for it.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

nodejs-github-bot · 2018-11-30T18:31:39Z

@lpinca build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1750/pipeline

lpinca · 2018-11-30T18:31:59Z

cc: @dougwilson

Destroy the socket if the `'clientError'` event is emitted and there is no listener for it. Fixes: nodejs#24586

addaleax · 2018-11-30T18:35:34Z

 if (this.writable) {
- this.end(badRequestResponse);
+ this.write(badRequestResponse);
+ this.destroy(e);


Would it make sense to wait for the write() callback to be called?

@addaleax @mcollina the assumption is that badRequestResponse is the first chunk that is written to the socket (it initiates the HTTP response). As such it is written synchronously so I think there is no need to wait for the write callback.

mcollina · 2018-11-30T18:36:43Z

 if (this.writable) {
- this.end(badRequestResponse);
+ this.write(badRequestResponse);
+ this.destroy(e);


We should give a chance to the event loop to write this. How about we put it into a setImmediate?

Using setImmediate() would make things even more race-y. I think it’s either using the write callback or not delaying it at all (which is still race-y, but probably more consistent?).

mcollina

LGTM

lpinca · 2018-12-01T09:26:20Z

CI: https://ci.nodejs.org/job/node-test-pull-request/19118/ ✔️

mcollina · 2018-12-01T09:39:16Z

Can you please verify that http://npm.im/on-finished tests pass with this patch?

lpinca · 2018-12-01T09:40:27Z

Will do later today.

lpinca · 2018-12-01T09:46:22Z

Btw we also need #24738 as we had a regression in v11.2.0 and there are cases where 'clientError' is not emitted.

lpinca · 2018-12-01T12:00:28Z

[luigi@fedora on-finished]$ ../dev/node/node node_modules/.bin/mocha
onFinished(res, listener)
✓ should invoke listener given an unknown object
when the response finishes
✓ should fire the callback
✓ should include the response object
✓ should fire when called after finish
when using keep-alive
✓ should fire for each response (45ms)
when requests pipelined
✓ should fire for each request
when response errors
✓ should fire with error
✓ should include the response object
when the response aborts
✓ should execute the callback
when calling many times on same response
✓ should not print warnings
isFinished(res)
✓ should return undefined for unknown object
✓ should be false before response finishes
✓ should be true after response finishes
when requests pipelined
✓ should have correct state when socket shared
✓ should handle aborted requests
when response errors
✓ should return true
when the response aborts
✓ should return true
onFinished(req, listener)
when the request finishes
✓ should fire the callback
✓ should include the request object
✓ should fire when called after finish
when using keep-alive
✓ should fire for each request (43ms)
when request errors
✓ should fire with error
✓ should include the request objecy
when the request aborts
✓ should execute the callback
when calling many times on same request
✓ should not print warnings
when CONNECT method
✓ should fire when request finishes
✓ should fire when called after finish
when Upgrade request
✓ should fire when request finishes
✓ should fire when called after finish
isFinished(req)
✓ should return undefined for unknown object
✓ should be false before request finishes
✓ should be true after request finishes
when request data buffered
✓ should be false before request finishes
when request errors
✓ should return true
when the request aborts
✓ should return true
when CONNECT method
✓ should be true immediately
✓ should be true after request finishes
when Upgrade request
✓ should be true immediately
✓ should be true after request finishes
39 passing (200ms)

Trott · 2018-12-04T02:02:28Z

Landed in ff7d053

Destroy the socket if the `'clientError'` event is emitted and there is no listener for it. Fixes: nodejs#24586 PR-URL: nodejs#24757 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Fishrock123 · 2018-12-04T02:28:33Z

Ah, nice fix.

MylesBorins · 2018-12-04T22:09:35Z

This lands cleanly on 10.x. How time sensitive is it to get this out in LTS?

lpinca · 2018-12-05T06:33:31Z

@MylesBorins I would say as soon as possible.

mcollina · 2018-12-05T12:27:52Z

We talked about it at today's @nodejs/tsc meeting and we think this fix should be released asap.

MylesBorins · 2018-12-05T14:32:12Z

/cc @codebytere please pick up this commit in the 10.x release

…

On Wed, Dec 5, 2018, 7:28 AM Matteo Collina ***@***.*** wrote: We talked about it at today's @nodejs/tsc <https://github.com/orgs/nodejs/teams/tsc> meeting and we think this fix should be released asap. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#24757 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV3-rtn_4KT6_G_PWC3cWXeC0ztPvks5u17vRgaJpZM4Y8LBu> .

Destroy the socket if the `'clientError'` event is emitted and there is no listener for it. Fixes: #24586 PR-URL: #24757 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Destroy the socket if the `'clientError'` event is emitted and there is no listener for it. Fixes: nodejs#24586 PR-URL: nodejs#24757 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Nov 30, 2018

http: destroy the socket on parse error
ed5b94a
Destroy the socket if the `'clientError'` event is emitted and there is no listener for it. Fixes: nodejs#24586

lpinca force-pushed the gh-24586 branch from f09c685 to ed5b94aCompare November 30, 2018 18:35

lpinca changed the title ~~http: destroy the socket on HTTP parse error~~http: destroy the socket on parse errorNov 30, 2018

addaleax approved these changes Nov 30, 2018
View reviewed changes

mcollina reviewed Nov 30, 2018
View reviewed changes

fixup! http: destroy the socket on parse error
7f03077

mcollina approved these changes Nov 30, 2018
View reviewed changes

lpinca mentioned this pull request Dec 1, 2018
meta: express.js internals do not support Node.js 10+ expressjs/express#3813
Closed

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 1, 2018

lpinca added dont-land-on-v6.x labels Dec 1, 2018

BridgeAR approved these changes Dec 1, 2018
View reviewed changes

Trott closed this Dec 4, 2018

Trott mentioned this pull request Dec 4, 2018
HTTP/1 request destroy behavior change on framing error #24586
Closed

lpinca deleted the gh-24586 branch December 4, 2018 06:26

BridgeAR mentioned this pull request Dec 5, 2018
v11.4.0 proposal #24854
Merged
4 tasks

MylesBorins added the land-on-v10.x label Dec 5, 2018

DRoet mentioned this pull request Dec 11, 2018
v10.14.2 proposal #24727
Merged

lpinca mentioned this pull request Feb 28, 2019
http: only destroy socket once error is sent #26356
Closed
4 tasks

Uh oh!

Uh oh!

Conversation

lpinca commented Nov 30, 2018

Checklist

Uh oh!

nodejs-github-bot commented Nov 30, 2018

Uh oh!

lpinca commented Nov 30, 2018

Uh oh!

addaleaxNov 30, 2018

Choose a reason for hiding this comment

Uh oh!

lpincaNov 30, 2018

Choose a reason for hiding this comment

Uh oh!

mcollinaNov 30, 2018

Choose a reason for hiding this comment

Uh oh!

addaleaxNov 30, 2018

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

lpinca commented Dec 1, 2018• edited by Trott Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Dec 1, 2018

Uh oh!

lpinca commented Dec 1, 2018

Uh oh!

lpinca commented Dec 1, 2018

Uh oh!

lpinca commented Dec 1, 2018

Uh oh!

Trott commented Dec 4, 2018

Uh oh!

Fishrock123 commented Dec 4, 2018

Uh oh!

MylesBorins commented Dec 4, 2018

Uh oh!

lpinca commented Dec 5, 2018

Uh oh!

mcollina commented Dec 5, 2018

Uh oh!

MylesBorins commented Dec 5, 2018 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

lpinca commented Dec 1, 2018•
edited by Trott
Loading