http: destroy the socket on parse error #24757

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

lib/_http_server.js

@@ -512,7 +512,8 @@ function socketOnError(e) {

  if (!this.server.emit('clientError', e, this)) {
    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

lib/_http_server.js

@@ -512,7 +512,8 @@ function socketOnError(e) {

  if (!this.server.emit('clientError', e, this)) {
    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 ed5b94a Compare November 30, 2018 18:35

lpinca changed the title ~~http: destroy the socket on HTTP parse error~~ http: destroy the socket on parse error Nov 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: destroy the socket on parse error #24757

http: destroy the socket on parse error #24757

lpinca commented Nov 30, 2018

nodejs-github-bot commented Nov 30, 2018

lpinca commented Nov 30, 2018

addaleax Nov 30, 2018

lpinca Nov 30, 2018

mcollina Nov 30, 2018

addaleax Nov 30, 2018

mcollina left a comment

lpinca commented Dec 1, 2018 •

edited by Trott

mcollina commented Dec 1, 2018

lpinca commented Dec 1, 2018

lpinca commented Dec 1, 2018

lpinca commented Dec 1, 2018

Trott commented Dec 4, 2018

Fishrock123 commented Dec 4, 2018

MylesBorins commented Dec 4, 2018

lpinca commented Dec 5, 2018

mcollina commented Dec 5, 2018

MylesBorins commented Dec 5, 2018 via email

http: destroy the socket on parse error #24757

http: destroy the socket on parse error #24757

Conversation

lpinca commented Nov 30, 2018

Checklist

nodejs-github-bot commented Nov 30, 2018

lpinca commented Nov 30, 2018

addaleax Nov 30, 2018

Choose a reason for hiding this comment

lpinca Nov 30, 2018

Choose a reason for hiding this comment

mcollina Nov 30, 2018

Choose a reason for hiding this comment

addaleax Nov 30, 2018

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

lpinca commented Dec 1, 2018 • edited by Trott

mcollina commented Dec 1, 2018

lpinca commented Dec 1, 2018

lpinca commented Dec 1, 2018

lpinca commented Dec 1, 2018

Trott commented Dec 4, 2018

Fishrock123 commented Dec 4, 2018

MylesBorins commented Dec 4, 2018

lpinca commented Dec 5, 2018

mcollina commented Dec 5, 2018

MylesBorins commented Dec 5, 2018 via email

lpinca commented Dec 1, 2018 •

edited by Trott