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

lpinca · 2017-01-15T14:00:17Z

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

jasnell · 2017-01-15T17:06:46Z

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

cjihrig · 2017-01-15T23:48:30Z

test/parallel/test-http-client-abort-no-agent.js

+const net = require('net');
+
+const server = http.createServer(() => {
+  throw new Error('test invalidation');


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.

bnoordhuis · 2017-01-16T04:41:26Z

lib/_http_client.js

+      socket.destroy();
+    } else {
+      socket.emit('free');
+    }


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.

lpinca · 2017-01-23T06:22:33Z

Is there anything I can do to move this forward?

bnoordhuis · 2017-01-23T06:33:31Z

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

lpinca · 2017-01-23T06:37:26Z

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

lpinca · 2017-01-23T06:57:10Z

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

lpinca · 2017-01-24T13:23:10Z

Also pinging other @nodejs/collaborators.

mcollina · 2017-01-24T13:27:30Z

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

bnoordhuis

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

mcollina

LGTM. CITGM should be checked as well

lpinca · 2017-01-26T07:22:56Z

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

lpinca · 2017-01-26T13:24:17Z

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.

gibfahn · 2017-01-26T13:45:40Z

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/

lpinca · 2017-01-26T13:53:58Z

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

lpinca · 2017-01-26T14:06:57Z

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.

lpinca · 2017-01-26T16:22:34Z

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.

lpinca · 2017-01-27T17:28:42Z

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

mcollina · 2017-01-27T17:47:13Z

@lpinca LGTM.

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

lpinca · 2017-01-27T17:52:48Z

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

lpinca · 2017-01-27T17:56:45Z

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

thefourtheye · 2017-01-27T18:41:45Z

test/parallel/test-http-abort-queued-2.js

+  const req = http.get({agent, port}, common.fail);
+  req.abort();
+
+  http.get({agent, port}, (res) => {


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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

lpinca · 2017-01-28T17:07:47Z

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

jasnell · 2017-03-08T23:18:49Z

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

`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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

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 #10818

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

Conversation

lpinca commented Jan 15, 2017

Checklist

Affected core subsystem(s)

jasnell commented Jan 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca Jan 24, 2017 • edited Loading

Choose a reason for hiding this comment

lpinca commented Jan 23, 2017

bnoordhuis commented Jan 23, 2017

lpinca commented Jan 23, 2017

lpinca commented Jan 23, 2017

lpinca commented Jan 24, 2017

mcollina commented Jan 24, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

lpinca commented Jan 26, 2017

lpinca commented Jan 26, 2017

gibfahn commented Jan 26, 2017

lpinca commented Jan 26, 2017

lpinca commented Jan 26, 2017 • edited Loading

lpinca commented Jan 26, 2017

lpinca commented Jan 27, 2017 • edited Loading

mcollina commented Jan 27, 2017

lpinca commented Jan 27, 2017

lpinca commented Jan 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca commented Jan 28, 2017

jasnell commented Mar 8, 2017

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

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

lpinca Jan 24, 2017 •

edited

Loading

lpinca commented Jan 26, 2017 •

edited

Loading

lpinca commented Jan 27, 2017 •

edited

Loading