http request fixes #20075

ronag · 2018-04-16T14:29:07Z

Fixes: #20102
Fixes: #20101
Fixes: #17352

Response should always emit close.
Response should always emit aborted if aborted.
Response should always emit close after request has emitted close, never before.

Checklist

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

ronag · 2018-04-23T16:46:10Z

@BridgeAR nice and clean PR 😃

jasnell · 2018-04-23T17:00:27Z

/cc @nodejs/http

apapirovski · 2018-04-23T16:59:53Z

lib/_http_client.js

-    var res = req.res;
-    res.on('end', function() {
+    if (!res.complete) {
+      res.aborted = Date.now();


Let's just make this true.

@apapirovski: I've got a separate PR for that #20230

Yeah, I saw that but I don't think this should land with it being a date. Date.now() is pretty slow so any aborted requests would kinda tank performance of the client. Perhaps let's leave it out for now and just have it land with the patch that makes it Boolean across the board?

apapirovski · 2018-04-23T16:59:56Z

lib/_http_client.js

-    res.push(null);
-  } else if (!req.res && !req.socket._hadError) {
+    }
+  } else if (!req.socket._hadError) {


I would probably slightly prefer this as:

else { if (!req.socket._hadError) { ... } req.emit('close'); }

apapirovski · 2018-04-23T17:05:00Z

lib/_http_client.js

-    if (!req.res.complete) req.res.emit('aborted');
-    var res = req.res;
-    res.on('end', function() {
+    if (!res.complete) {


I'm not sure if the fact that it's no longer within res.readable is meaningful here or not. I just need to think this over quickly but the rest of this looks good.

apapirovski · 2018-04-23T17:05:37Z

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

mcollina · 2018-04-24T23:18:43Z

Can you please include the relative Fixes: <LINK> in the commit message and PR description?

mcollina

LGTM with CI and CITGM ok

ronag · 2018-04-25T04:52:39Z

@mcollina fixed

BridgeAR · 2018-04-25T11:40:51Z

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

BridgeAR · 2018-04-25T11:41:55Z

@nodejs/http @nodejs/http2 PTAL

mafintosh · 2018-04-26T11:22:21Z

👎 This seems wrong to me. You should not be emitting close yourself, destroy will do that for you. Doing this manually will break things and might lead to double close events.

mafintosh · 2018-04-26T11:24:32Z

If we want to make streams emit close in general as the last event it it's a very different thing with much bigger implications. That should be done in the streams code itself (always call .destroy internally after end/finish). I've made a discussion issue for that a few weeks ago #20096

apapirovski · 2018-04-26T11:27:38Z

If we want to make streams emit close in general as the last event it it's a very different thing with much bigger implications. That should be done in the streams code itself (always call .destroy internally after end/finish). I've made a discussion issue for that a few weeks ago #20096

It's already emitting it. This just makes the behaviour more predictable. Have a look at the diff.

Edit: And this isn't blocked.

mafintosh · 2018-04-26T11:33:44Z

In either case this should really use .destroy instead.

mafintosh · 2018-04-26T11:39:09Z

If people really want to merge as is I can live with it, but we are should be improving the streams error handling in core to use destroy in general when we do updates imo

apapirovski · 2018-04-26T11:39:16Z

In either case this should really use .destroy instead.

OutgoingMessage doesn't even inherit from Writable. And we never even call destroy on IncomingMessage, except for apparently in pipeline which was recently added to the project.

but we should be improving the streams error handling in core to use destroy in general when we do updates imo

Someone would need to make a PR. It's been this way for years and no one is exactly rushing to do it.

This improves the situation even if it's still not ideal.

Fixes: #20102 Fixes: #20101 Fixes: #1735 - Response should always emit close. - Response should always emit aborted if aborted. - Response should always emit close after request has emitted close. PR-URL: #20075 Fixes: #17352 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

Notable Changes: * **addons**: - Fixed a memory leak for users of `AsyncResource` and N-API. (Michael Dawson) [#20668](#20668) * **assert**: - The `error` parameter of `assert.throws()` can be an object containing regular expressions now. (Ruben Bridgewater) [#20485](#20485) * **crypto**: - The `authTagLength` option has been made more flexible. (Tobias Nießen) [#20235](#20235), [#20039](#20039) * **http**: - Handling of `close` and `aborted` events has been made more consistent. (Robert Nagy) [#20075](#20075), [#20611](#20611) * Embedder support: - Functions for creating V8 `Isolate` and `Context` objects with Node.js-specific behaviour have been added to the API. (helloshuangzi) [#20639](#20639) - Node.js `Environment`s clean up resources before exiting now. (Anna Henningsen) [#19377](#19377) - Support for multi-threaded embedding has been improved. (Anna Henningsen) [#20542](#20542), [#20539](#20539), [#20541](#20541) * **timers**: - `timeout.refresh()` has been added to the public API. (Jeremiah Senkpiel) [#20298](#20298) PR-URL: #20724

Notable Changes: * **addons**: - Fixed a memory leak for users of `AsyncResource` and N-API. (Michael Dawson) [#20668](#20668) * **assert**: - The `error` parameter of `assert.throws()` can be an object containing regular expressions now. (Ruben Bridgewater) [#20485](#20485) * **crypto**: - The `authTagLength` option has been made more flexible. (Tobias Nießen) [#20235](#20235), [#20039](#20039) * **http**: - Handling of `close` and `aborted` events has been made more consistent. (Robert Nagy) [#20075](#20075), [#20611](#20611) * Embedder support: - Functions for creating V8 `Isolate` and `Context` objects with Node.js-specific behaviour have been added to the API. (helloshuangzi) [#20639](#20639) - Node.js `Environment`s clean up resources before exiting now. (Anna Henningsen) [#19377](#19377) - Support for multi-threaded embedding has been improved. (Anna Henningsen) [#20542](#20542), [#20539](#20539), [#20541](#20541) * **esm**: - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules. (Gus Caplan) [#20403](#20403) * **timers**: - `timeout.refresh()` has been added to the public API. (Jeremiah Senkpiel) [#20298](#20298) PR-URL: #20724

Notable Changes: * **addons**: - Fixed a memory leak for users of `AsyncResource` and N-API. (Michael Dawson) [#20668](#20668) * **assert**: - The `error` parameter of `assert.throws()` can be an object containing regular expressions now. (Ruben Bridgewater) [#20485](#20485) * **crypto**: - The `authTagLength` option has been made more flexible. (Tobias Nießen) [#20235](#20235), [#20039](#20039) * **http**: - Handling of `close` and `aborted` events has been made more consistent. (Robert Nagy) [#20075](#20075), [#20611](#20611) * Embedder support: - Functions for creating V8 `Isolate` and `Context` objects with Node.js-specific behaviour have been added to the API. (Allen Yonghuang Wang) [#20639](#20639) - Node.js `Environment`s clean up resources before exiting now. (Anna Henningsen) [#19377](#19377) - Support for multi-threaded embedding has been improved. (Anna Henningsen) [#20542](#20542), [#20539](#20539), [#20541](#20541) * **esm**: - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules. (Gus Caplan) [#20403](#20403) * **timers**: - `timeout.refresh()` has been added to the public API. (Jeremiah Senkpiel) [#20298](#20298) PR-URL: #20724

* addons: - Fixed a memory leak for users of `AsyncResource` and N-API. (Michael Dawson) #20668 * assert: - The `error` parameter of `assert.throws()` can be an object containing regular expressions now. (Ruben Bridgewater) #20485 * crypto: - The `authTagLength` option has been made more flexible (Tobias Nießen) #20235) #20039 * esm: - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules. (Gus Caplan) #20403 * http: - Handling of `close` and `aborted` events has been made more consistent. (Robert Nagy) #20075 #20611 * module: - add --preserve-symlinks-main (David Goldstein) #19911 * timers: - `timeout.refresh()` has been added to the public API. (Jeremiah Senkpiel) #20298 * Embedder support: - Functions for creating V8 `Isolate` and `Context` objects with Node.js-specific behaviour have been added to the API. (Allen Yonghuang Wang) #20639 - Node.js `Environment`s clean up resources before exiting now. (Anna Henningsen) #19377 - Support for multi-threaded embedding has been improved. (Anna Henningsen) #20542 #20539 #20541 PR-URL: #20724

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

ronag changed the title ~~always emit 'aborted' after 'response' and consistent 'close' order~~ http request fixes Apr 16, 2018

This was referenced Apr 16, 2018

http: response should always emit close on finish #20043

Closed

http: client request req & res 'close' order is inconsistent #20102

Closed

http: client request doesn't always emit aborted when aborted #20101

Closed

ronag force-pushed the http-client-fix branch from 8ad8157 to a432a60 Compare April 23, 2018 16:44

apapirovski reviewed Apr 23, 2018

View reviewed changes

ronag force-pushed the http-client-fix branch 3 times, most recently from 08fabbd to ee28236 Compare April 23, 2018 17:41

mcollina approved these changes Apr 24, 2018

View reviewed changes

ronag force-pushed the http-client-fix branch from ee28236 to afa3ffb Compare April 25, 2018 04:52

ronag force-pushed the http-client-fix branch from afa3ffb to aee444a Compare April 25, 2018 04:55

jasnell approved these changes Apr 25, 2018

View reviewed changes

trivikr approved these changes Apr 25, 2018

View reviewed changes

BridgeAR added the blocked PRs that are blocked by other issues or PRs. label Apr 26, 2018

apapirovski removed the blocked PRs that are blocked by other issues or PRs. label Apr 26, 2018

trivikr closed this May 14, 2018

addaleax mentioned this pull request May 14, 2018

v10.2.0 proposal #20724

Merged

sebastien-cleany mentioned this pull request May 28, 2018

message.sendSms - TypeError: Cannot read property 'status' of undefined Vonage/vonage-node-sdk#188

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http request fixes #20075

http request fixes #20075

ronag commented Apr 16, 2018 •

edited

Loading

ronag commented Apr 23, 2018

jasnell commented Apr 23, 2018

apapirovski Apr 23, 2018

ronag Apr 23, 2018

apapirovski Apr 23, 2018

ronag Apr 23, 2018

apapirovski Apr 23, 2018

apapirovski Apr 23, 2018

apapirovski commented Apr 23, 2018

mcollina commented Apr 24, 2018

mcollina left a comment

ronag commented Apr 25, 2018

BridgeAR commented Apr 25, 2018

BridgeAR commented Apr 25, 2018

mafintosh commented Apr 26, 2018

mafintosh commented Apr 26, 2018

apapirovski commented Apr 26, 2018 •

edited

Loading

mafintosh commented Apr 26, 2018

mafintosh commented Apr 26, 2018

apapirovski commented Apr 26, 2018 •

edited

Loading

http request fixes #20075

http request fixes #20075

Conversation

ronag commented Apr 16, 2018 • edited Loading

ronag commented Apr 23, 2018

jasnell commented Apr 23, 2018

apapirovski Apr 23, 2018

Choose a reason for hiding this comment

ronag Apr 23, 2018

Choose a reason for hiding this comment

apapirovski Apr 23, 2018

Choose a reason for hiding this comment

ronag Apr 23, 2018

Choose a reason for hiding this comment

apapirovski Apr 23, 2018

Choose a reason for hiding this comment

apapirovski Apr 23, 2018

Choose a reason for hiding this comment

apapirovski commented Apr 23, 2018

mcollina commented Apr 24, 2018

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Apr 25, 2018

BridgeAR commented Apr 25, 2018

BridgeAR commented Apr 25, 2018

mafintosh commented Apr 26, 2018

mafintosh commented Apr 26, 2018

apapirovski commented Apr 26, 2018 • edited Loading

mafintosh commented Apr 26, 2018

mafintosh commented Apr 26, 2018

apapirovski commented Apr 26, 2018 • edited Loading

ronag commented Apr 16, 2018 •

edited

Loading

apapirovski commented Apr 26, 2018 •

edited

Loading

apapirovski commented Apr 26, 2018 •

edited

Loading