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 request fixes #20075

Closed
wants to merge 1 commit into from
Closed

http request fixes #20075

wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Apr 16, 2018

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

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Apr 16, 2018
@ronag ronag changed the title always emit 'aborted' after 'response' and consistent 'close' order http request fixes Apr 16, 2018
@ronag
Copy link
Member Author

ronag commented Apr 23, 2018

@BridgeAR nice and clean PR 😃

@jasnell
Copy link
Member

jasnell commented Apr 23, 2018

/cc @nodejs/http

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

Choose a reason for hiding this comment

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

Let's just make this true.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

res.push(null);
} else if (!req.res && !req.socket._hadError) {
}
} else if (!req.socket._hadError) {
Copy link
Member

Choose a reason for hiding this comment

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

I would probably slightly prefer this as:

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

if (!req.res.complete) req.res.emit('aborted');
var res = req.res;
res.on('end', function() {
if (!res.complete) {
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

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

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

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 with CI and CITGM ok

@ronag
Copy link
Member Author

ronag commented Apr 25, 2018

@mcollina fixed

@BridgeAR
Copy link
Member

@BridgeAR
Copy link
Member

@nodejs/http @nodejs/http2 PTAL

@mafintosh
Copy link
Member

👎 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
Copy link
Member

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

@BridgeAR BridgeAR added the blocked PRs that are blocked by other issues or PRs. label Apr 26, 2018
@apapirovski
Copy link
Member

apapirovski commented Apr 26, 2018

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.

@apapirovski apapirovski removed the blocked PRs that are blocked by other issues or PRs. label Apr 26, 2018
@mafintosh
Copy link
Member

In either case this should really use .destroy instead.

@mafintosh
Copy link
Member

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
Copy link
Member

apapirovski commented Apr 26, 2018

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.

@trivikr trivikr closed this May 14, 2018
trivikr pushed a commit that referenced this pull request May 14, 2018
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
addaleax pushed a commit that referenced this pull request May 14, 2018
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@addaleax addaleax mentioned this pull request May 14, 2018
addaleax added a commit that referenced this pull request May 14, 2018
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
addaleax added a commit that referenced this pull request May 15, 2018
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
addaleax added a commit that referenced this pull request May 15, 2018
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
MylesBorins pushed a commit that referenced this pull request May 22, 2018
* 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
MylesBorins pushed a commit that referenced this pull request May 23, 2018
* 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
MylesBorins pushed a commit that referenced this pull request May 23, 2018
* 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
MylesBorins pushed a commit that referenced this pull request May 23, 2018
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
8 participants