Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: destroy the socket on parse error #24757

Closed
wants to merge 2 commits into from
Closed

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Nov 30, 2018

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

Fixes: #24586

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

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Nov 30, 2018
@lpinca
Copy link
Member Author

lpinca commented Nov 30, 2018

cc: @dougwilson

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

Fixes: nodejs#24586
@lpinca lpinca changed the title http: destroy the socket on HTTP parse error http: destroy the socket on parse error Nov 30, 2018
@@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?).

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lpinca
Copy link
Member Author

lpinca commented Dec 1, 2018

@mcollina
Copy link
Member

mcollina commented Dec 1, 2018

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

@lpinca
Copy link
Member Author

lpinca commented Dec 1, 2018

Will do later today.

@lpinca
Copy link
Member Author

lpinca commented Dec 1, 2018

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

@lpinca
Copy link
Member Author

lpinca commented Dec 1, 2018

[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 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
@Trott
Copy link
Member

Trott commented Dec 4, 2018

Landed in ff7d053

@Trott Trott closed this Dec 4, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Dec 4, 2018
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
Copy link
Contributor

Ah, nice fix.

@lpinca lpinca deleted the gh-24586 branch December 4, 2018 06:26
@MylesBorins
Copy link
Contributor

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

@lpinca
Copy link
Member Author

lpinca commented Dec 5, 2018

@MylesBorins I would say as soon as possible.

@mcollina
Copy link
Member

mcollina commented Dec 5, 2018

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

@MylesBorins
Copy link
Contributor

MylesBorins commented Dec 5, 2018 via email

BridgeAR pushed a commit that referenced this pull request Dec 5, 2018
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>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
4 tasks
MylesBorins pushed a commit that referenced this pull request Dec 5, 2018
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>
@DRoet DRoet mentioned this pull request Dec 11, 2018
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP/1 request destroy behavior change on framing error
8 participants