http: make `request.abort()` destroy the socket by lpinca · Pull Request #10818 · nodejs/node · GitHub

request.abort() did not destroy the socket if it was called
before a socket was assigned to the request and the request
did not use an Agent or a Unix Domain Socket was used.

Fixes: #10812

Checklist

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

Affected core subsystem(s)

http

I think this is fine, but let's get some good review @nodejs/http @indutny @mscdex

Can you use common.fail() instead of explicitly throwing (in the other test too).

I originally used common.fail() but I honestly don't like to use it with a non string argument.
In this case it will be called with the request object and the error message will be something like AssertionError: [object Object]. I find it horrible.

common.fail() takes an optional string argument.

@cjihrig are you suggesting to use this?

http.createServer(() => common.fail('whatever'));

I thought you wanted to pass it directly to http.createServer()

http.createServer(common.fail);

I'd be OK with either. If you like the first one, go with that.

Even if I don't like http.createServer(common.fail);, for the reason stated above, I changed it to that for consistency as there are other tests which use the same pattern.

I'm not even sure if it's appropriate to emit 'free' and skip destroying the socket. ClientRequest#abort() always calls socket.destroy() when there is one.

@bnoordhuis I think this is for keep-alive sockets?

@bnoordhuis I guess you were referring to

node/lib/_http_client.js

Lines 296 to 299 in 77be180

    
           if (this.socket) { 
        
             // in-progress 
        
             this.socket.destroy(); 
        
           }

. I'm not sure either.

This is when socket.emit('free'); was added: d0c7d93#diff-e3bc37430eb078ccbafe3aa3b570c91a, previously the socket was destroyed in all cases.

I think we should always destroy(), I do not see a way for this socket to be reused even if there is an agent.

Agree. There's really no safe way to reuse it. destroy() is the safest thing to do

I've added a test which should clarify why socket.emit('free') is used. It's for queued requests which are already aborted.

See f52c874b737ccb7e92d640b31e02c6efb3acd22c.

Is there anything I can do to move this forward?

@lpinca Did you find an answer to https://github.com/nodejs/node/pull/10818/files#r96164070?

@bnoordhuis yeah, this commit d0c7d93#diff-e3bc37430eb078ccbafe3aa3b570c91a

Avoid sending unsent data and destroying otherwise legitimate sockets
for requests that are aborted while still in the agent queue. This
reduces stress on upstream systems who will likely respond to the
request but client app already knows that it will be dropped on the
floor and also helps avoid killing keep-alive connections.

The original PR doesn't seem to give more info: nodejs/node-v0.x-archive#7533

Also pinging other @nodejs/collaborators.

CI: https://ci.nodejs.org/job/node-test-pull-request/6026/

LGTM with some trepidation. Good job on the test coverage though.

LGTM. CITGM should be checked as well

CI: https://ci.nodejs.org/job/node-test-pull-request/6065/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/546/

I think CITGM failures (express and spdy) are not related to this change as I can also reproduce them with current master. browserify also failed in other CITGM runs.

The browserify failure is a module issue (nodejs/citgm#335), not sure about express and spdy.

There's also a failure on ember-cli: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/546/nodes=ppcle-ubuntu1404/testReport/(root)/citgm/ember_cli_v2_11_0/

Yup but it doesn't seem to use abort() at all, not sure.

I've started one more CITGM run against master to see if it fails with express and spdy. If not I'll rebase this and run again.

There seem to be quite a bit of failures with master https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/547.

I'll wait a bit before rerunning against this PR.

#10558 is responsible for CITGM failures with express and spdy, so I've rebased this before #10558 was landed and re-ran

CI: https://ci.nodejs.org/job/node-test-pull-request/6079/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/549/

There is one CITGM suspicious failure on ppcle-ubuntu1404 with spdy but I think it is not related as the failing test fails for spdy/3.1 but not for h2 and spdy/3. On top of this the test seems to not use request.abort() at all.

@lpinca LGTM.

I'm not 100% sure we should backport this to the LTS releases, but that's another discussion.

It's a bug-fix and I hope we can backport this. It's needed for ws where I found the issue.

If there are no objections I'll land this tomorrow.

As we have an assertion within this, I would wrap this in common.mustCall

I didn't add it as the test will fail for timeout if the function is not called but will add it if desired.

@thefourtheye done.

`request.abort()` did not destroy the socket if it was called before a socket was assigned to the request and the request did not use an `Agent` or a Unix Domain Socket was used. Fixes: nodejs#10812 PR-URL: nodejs#10818 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Landed in 18d4ee9.

`request.abort()` did not destroy the socket if it was called before a socket was assigned to the request and the request did not use an `Agent` or a Unix Domain Socket was used. Fixes: #10812 PR-URL: #10818 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

The tests fail with timeouts on v4.x. Will need a backport PR to land there.
Landing on v6

`request.abort()` did not destroy the socket if it was called before a socket was assigned to the request and the request did not use an `Agent` or a Unix Domain Socket was used. Fixes: #10812 PR-URL: #10818 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

`request.abort()` did not destroy the socket if it was called before a socket was assigned to the request and the request did not use an `Agent` or a Unix Domain Socket was used. Fixes: nodejs/node#10812 PR-URL: nodejs/node#10818 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

nodejs-github-bot added http Issues or PRs related to the http subsystem. lts-watch-v6.x labels Jan 15, 2017

cjihrig reviewed Jan 15, 2017

View reviewed changes

bnoordhuis reviewed Jan 16, 2017

View reviewed changes

nodejs-github-bot added lts-watch-v6.x labels Jan 16, 2017

lpinca mentioned this pull request Jan 19, 2017

doc: update http.md for consistency #10715

Closed

2 tasks

nodejs-github-bot added lts-watch-v6.x labels Jan 24, 2017

bnoordhuis approved these changes Jan 24, 2017

View reviewed changes

mcollina approved these changes Jan 26, 2017

View reviewed changes

lpinca force-pushed the fix/request-abort branch from f52c874 to 11648e0 Compare January 27, 2017 15:28

nodejs-github-bot added lts-watch-v6.x labels Jan 27, 2017

thefourtheye reviewed Jan 27, 2017

View reviewed changes

nodejs-github-bot added lts-watch-v6.x labels Jan 27, 2017

lpinca force-pushed the fix/request-abort branch from bab407d to b0ac117 Compare January 28, 2017 17:02

nodejs-github-bot added lts-watch-v6.x labels Jan 28, 2017

lpinca force-pushed the fix/request-abort branch from b0ac117 to 18d4ee9 Compare January 28, 2017 17:06

nodejs-github-bot added lts-watch-v6.x labels Jan 28, 2017

lpinca closed this Jan 28, 2017

lpinca merged commit 18d4ee9 into nodejs:master Jan 28, 2017

lpinca deleted the fix/request-abort branch January 28, 2017 17:08

italoacasas mentioned this pull request Jan 31, 2017

v7.5.0 proposal #11062

Merged

jasnell added the dont-land-on-v4.x label Mar 8, 2017

MylesBorins mentioned this pull request Mar 9, 2017

v6.10.1 proposal #11759

Merged

lpinca mentioned this pull request Mar 9, 2017

v4.x backport: http: make request.abort() destroy the socket #11765

Closed

MylesBorins mentioned this pull request Mar 11, 2017

v4.8.1 proposal #11760

Merged

lpinca mentioned this pull request May 25, 2017

http: client keep-alive for UNIX domain sockets #13214

Closed

3 tasks

http: make `request.abort()` destroy the socket by lpinca · Pull Request #10818 · nodejs/node · GitHub

Uh oh!

Uh oh!

http: make request.abort() destroy the socket - #10818

http: make request.abort() destroy the socket#10818lpinca merged 1 commit intonodejs:masternodejs/node:masterfrom lpinca:fix/request-abortlpinca/node:fix/request-abortCopy head branch name to clipboard

Conversation

lpinca commented Jan 15, 2017

Checklist

Affected core subsystem(s)

Uh oh!

jasnell commented Jan 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca Jan 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca commented Jan 23, 2017

Uh oh!

bnoordhuis commented Jan 23, 2017

Uh oh!

lpinca commented Jan 23, 2017

Uh oh!

lpinca commented Jan 23, 2017

Uh oh!

lpinca commented Jan 24, 2017

Uh oh!

mcollina commented Jan 24, 2017

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

lpinca commented Jan 26, 2017

Uh oh!

lpinca commented Jan 26, 2017

Uh oh!

gibfahn commented Jan 26, 2017

Uh oh!

lpinca commented Jan 26, 2017

Uh oh!

lpinca commented Jan 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpinca commented Jan 26, 2017

Uh oh!

lpinca commented Jan 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Jan 27, 2017

Uh oh!

lpinca commented Jan 27, 2017

Uh oh!

http: make request.abort() destroy the socket#10818
lpinca merged 1 commit into
nodejs:masterfrom
lpinca:fix/request-abort

lpinca Jan 24, 2017 •
edited
Loading

lpinca commented Jan 26, 2017 •
edited
Loading

lpinca commented Jan 27, 2017 •
edited
Loading