http: prevent aborted event when request is complete by billywhizz · Pull Request #18999 · nodejs/node

billywhizz · 2018-02-26T12:54:56Z

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

Affected core subsystem(s)

http

When socket is closed on a response for a request that is being piped to a stream
there is a condition where aborted event will be fired to http client when socket is
closing and the incomingMessage stream is still set to readable.

We need a check for request being complete and to only raise the 'aborted' event
on the http client if we have not yet completed reading the response from the server.

When socket is closed on a response for a request that is being piped to a stream there is a condition where aborted event will be fired to http client when socket is closing and the incomingMessage stream is still set to readable. We need a check for request being complete and to only raise the 'aborted' event on the http client if we have not yet completed reading the response from the server. Fixes: nodejs#18756

Tests in progress to reproduce issue consistently. Fixes: nodejs#18756

mscdex · 2018-02-26T13:12:23Z

test/parallel/test-http-client-spurious-aborted.js

+  headers['Content-Length'] = 50;
+  const socket = res.socket;
+  res.writeHead(200, headers);
+  setTimeout(() => res.write('aaaaaaaaaa'), 100);


Shouldn't all of these timers be setTimeout(..., common.platformTimeout(n)) instead? Otherwise, is there a more reliable way to trigger this issue (perhaps using nextTick() and/or setImmediate() or something else)?

mcollina

Good work! The change itself is fine, I've left some notes on the test.

mcollina · 2018-02-26T13:19:59Z

test/parallel/test-http-client-spurious-aborted.js

+    res.pipe(fstream);
+    const _handle = res.socket._handle;
+    _handle._close = res.socket._handle.close;
+    _handle.close = function(callback) {


Can you explain why are you overriding this here?

because i could not find a way of forcing the condition we get intermittently in the wild. if you run this script and leave it it will eventually start aborting requests that are actually complete: https://gist.github.com/billywhizz/b500a0d4708f89625ddbb313601b5363. i was unable to figure out how to recreate those exact conditions in a test but after debugging i could see the aborted event was being fired even though req.res.complete was = true. so i overrode close here to recreate that condition.

to clarify, the error occurs when req.res.complete = true and res.readable is still = true. that is happening in the wild but don't know how to force it for a test. i imagine it will depend on a specific sequence of TCP session interactions.

mcollina · 2018-02-26T13:23:19Z

test/parallel/test-http-client-spurious-aborted.js

+const finishCountdown = new Countdown(N, common.mustCall(() => {
+  server.close();
+}));
+const reqCountdown = new Countdown(N, common.mustCall());


I would prefer if N was not a constant, but rather it uses a two steps approach, maybe with some helpers.

It's not clear why you are calling twice too.

it's running two tests - one to ensure we don't abort a good request, one to ensure we do abort a bad one. i can refactor to take out the countdown timers altogether. was trying to stay close to how other tests are constructed. if you can suggest how to change i am happy to refactor.

Leko · 2018-02-26T14:55:17Z

test/parallel/test-http-client-spurious-aborted.js

+    assert.strictEqual(res.statusCode, 200);
+    assert.strictEqual(res.headers.connection, 'close');
+    let aborted = false;
+    const fstream = fs.createWriteStream(fname);


I think this file can be made more simple.
No need to cleanup and fs module.

const { Writable } = require('stream'); // ... const writable = new Writable({ write(chunk, encoding, callback) { callback(); } }); res.pipe(writable); // ... writable.on('finish', () => {

billywhizz · 2018-02-27T01:19:45Z

I've made changes suggested above by @Leko and @mscdex . @mcollina if you want me to make any further changes to the test let me know. it would be good if we could figure out how to recreate the exact error condition without having to override handle._close.

mcollina · 2018-02-27T12:09:03Z

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

Let's see what CI and CITGM think.

mcollina

LGTM

billywhizz · 2018-03-01T15:03:24Z

let me know if there is anything else i need to do guys. thx

Leko · 2018-03-01T16:10:06Z

Landed in 3828fc6...38eb0fa
Thanks!

When socket is closed on a response for a request that is being piped to a stream there is a condition where aborted event will be fired to http client when socket is closing and the incomingMessage stream is still set to readable. We need a check for request being complete and to only raise the 'aborted' event on the http client if we have not yet completed reading the response from the server. Fixes: #18756 PR-URL: #18999 Reviewed-By: Shingo Inoue <leko.noor@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Tests in progress to reproduce issue consistently. Fixes: #18756 PR-URL: #18999 Reviewed-By: Shingo Inoue <leko.noor@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #18999 Reviewed-By: Shingo Inoue <leko.noor@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

lpinca · 2018-03-01T17:17:26Z

@Leko please squash commits next time before landing.

Leko · 2018-03-03T12:00:17Z

@lpinca I got it. I'm sorry for mistaken.

lpinca · 2018-03-03T14:43:48Z

No problem.

When socket is closed on a response for a request that is being piped to a stream there is a condition where aborted event will be fired to http client when socket is closing and the incomingMessage stream is still set to readable. We need a check for request being complete and to only raise the 'aborted' event on the http client if we have not yet completed reading the response from the server. Fixes: nodejs#18756 PR-URL: nodejs#18999 Reviewed-By: Shingo Inoue <leko.noor@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Tests in progress to reproduce issue consistently. Fixes: nodejs#18756 PR-URL: nodejs#18999 Reviewed-By: Shingo Inoue <leko.noor@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#18999 Reviewed-By: Shingo Inoue <leko.noor@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

When socket is closed on a response for a request that is being piped to a stream there is a condition where aborted event will be fired to http client when socket is closing and the incomingMessage stream is still set to readable. We need a check for request being complete and to only raise the 'aborted' event on the http client if we have not yet completed reading the response from the server. Fixes: nodejs#18756 PR-URL: nodejs#18999 Reviewed-By: Shingo Inoue <leko.noor@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Tests in progress to reproduce issue consistently. Fixes: nodejs#18756 PR-URL: nodejs#18999 Reviewed-By: Shingo Inoue <leko.noor@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#18999 Reviewed-By: Shingo Inoue <leko.noor@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

When socket is closed on a response for a request that is being piped to a stream there is a condition where aborted event will be fired to http client when socket is closing and the incomingMessage stream is still set to readable. We need a check for request being complete and to only raise the 'aborted' event on the http client if we have not yet completed reading the response from the server. Fixes: nodejs#18756 PR-URL: nodejs#18999 Reviewed-By: Shingo Inoue <leko.noor@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Tests in progress to reproduce issue consistently. Fixes: nodejs#18756 PR-URL: nodejs#18999 Reviewed-By: Shingo Inoue <leko.noor@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#18999 Reviewed-By: Shingo Inoue <leko.noor@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

When socket is closed on a response for a request that is being piped to a stream there is a condition where aborted event will be fired to http client when socket is closing and the incomingMessage stream is still set to readable. We need a check for request being complete and to only raise the 'aborted' event on the http client if we have not yet completed reading the response from the server. Fixes: #18756 Backport-PR-URL: #22380 PR-URL: #18999 Reviewed-By: Shingo Inoue <leko.noor@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Tests in progress to reproduce issue consistently. Fixes: #18756 Backport-PR-URL: #22380 PR-URL: #18999 Reviewed-By: Shingo Inoue <leko.noor@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Backport-PR-URL: #22380 PR-URL: #18999 Reviewed-By: Shingo Inoue <leko.noor@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

billywhizz added 3 commits February 17, 2018 19:05

http: prevent aborted event when request is complete

68850ef

Tests in progress to reproduce issue consistently. Fixes: nodejs#18756

working

cfc03aa

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

billywhizz mentioned this pull request Feb 26, 2018

HTTP Client 'aborted' event fires even when request was ok #18756

Closed

mscdex reviewed Feb 26, 2018

View reviewed changes

mcollina reviewed Feb 26, 2018

View reviewed changes

Leko reviewed Feb 26, 2018

View reviewed changes

fix(test): refactor test after review

24e5755

Leko approved these changes Feb 28, 2018

View reviewed changes

mcollina approved these changes Feb 28, 2018

View reviewed changes

jasnell approved these changes Feb 28, 2018

View reviewed changes

Leko closed this Mar 1, 2018

Leko pushed a commit that referenced this pull request Mar 1, 2018

test: refactor test after review

38eb0fa

PR-URL: #18999 Reviewed-By: Shingo Inoue <leko.noor@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins mentioned this pull request Mar 6, 2018

v9.8.0 proposal #19181

Merged

MylesBorins mentioned this pull request Sep 6, 2018

v8.12.0 proposal #21593

Merged

Uh oh!

Conversation

billywhizz commented Feb 26, 2018

Affected core subsystem(s)

Uh oh!

mscdex Feb 26, 2018 • 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!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina Feb 26, 2018

Choose a reason for hiding this comment

Uh oh!

billywhizz Feb 27, 2018

Choose a reason for hiding this comment

Uh oh!

billywhizz Feb 27, 2018

Choose a reason for hiding this comment

Uh oh!

mcollina Feb 26, 2018

Choose a reason for hiding this comment

Uh oh!

billywhizz Feb 27, 2018

Choose a reason for hiding this comment

Uh oh!

Leko Feb 26, 2018

Choose a reason for hiding this comment

Uh oh!

billywhizz commented Feb 27, 2018

Uh oh!

mcollina commented Feb 27, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

billywhizz commented Mar 1, 2018

Uh oh!

Leko commented Mar 1, 2018

Uh oh!

lpinca commented Mar 1, 2018

Uh oh!

Leko commented Mar 3, 2018

Uh oh!

lpinca commented Mar 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mscdex Feb 26, 2018 •

edited

Loading