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.

Sign up for GitHub

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

Jump to bottom

http: always emit close on req and res #20611

Closed
wants to merge 1 commit into from
Closed

http: always emit close on req and res #20611

wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented May 8, 2018 •
edited
Loading

'close' should always be emitted on req and res.

Fixes: #20600

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 May 8, 2018
@ronag ronag changed the title Fix emit close req res http: fix emit close req res May 8, 2018
@ronag ronag force-pushed the fix-emit-close-req-res branch 4 times, most recently from f389c78 to 8ce8826 Compare May 8, 2018 20:00
@ronag ronag changed the title http: fix emit close req res http: always emit close on req and res May 8, 2018
@mscdex
Copy link
Contributor

mscdex commented May 8, 2018

Perhaps the test filename should also include the keyword 'req' since both are being tested?

@ronag ronag force-pushed the fix-emit-close-req-res branch from 8ce8826 to 30896bc Compare May 9, 2018 09:15
@mscdex
Copy link
Contributor

mscdex commented May 9, 2018

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

@trivikr
Copy link
Member

trivikr commented May 13, 2018

New CI before landing https://ci.nodejs.org/job/node-test-pull-request/14847/

trivikr pushed a commit that referenced this pull request May 13, 2018
@ronag @trivikr
http: always emit close on req and res
f22c7c1 
PR-URL: #20611
Fixes: #20600
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@trivikr
Copy link
Member

trivikr commented May 13, 2018

Landed in f22c7c1

@trivikr trivikr closed this May 13, 2018
addaleax pushed a commit that referenced this pull request May 14, 2018
@ronag @addaleax
http: always emit close on req and res
8029a24 
PR-URL: #20611
Fixes: #20600
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@addaleax addaleax mentioned this pull request May 14, 2018
Merged
addaleax added a commit that referenced this pull request May 14, 2018
@addaleax
2018-05-15, Version 10.2.0 (Current), @???
3b86468 
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
@addaleax
2018-05-15, Version 10.2.0 (Current), @???
411ecf2 
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
@addaleax
2018-05-15, Version 10.2.0 (Current), @???
d0eec7b 
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
@addaleax @MylesBorins
2018-05-23, Version 10.2.0 (Current)
d765fb6 
* 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
@addaleax @MylesBorins
2018-05-23, Version 10.2.0 (Current)
f6b6b3f 
* 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
@addaleax @MylesBorins
2018-05-23, Version 10.2.0 (Current)
5cbb905 
* 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
@addaleax @MylesBorins
2018-05-23, Version 10.2.0 (Current)
82f1811 
* 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
msmol pushed a commit to msmol/bunyan-middleware that referenced this pull request Jun 1, 2018
No longer warns on res.close event
e86dc99 
See: nodejs/node#20611

res.close is always triggered and is no longer considered and error
or warning condition
@msmol msmol mentioned this pull request Jun 1, 2018
Merged
hekike pushed a commit to restify/node-restify that referenced this pull request Jun 6, 2018
fix(restify): handle res close event in Node >= 10
929f9a1 
Changes introduced in:
nodejs/node#20941
nodejs/node#20611
@hekike hekike mentioned this pull request Jun 6, 2018
Merged
@mareksrom
Copy link

I think this change is incosistent with the documentation of close event - "Indicates that the underlying connection was terminated before response.end() was called or able to flush." - this is not true anymore - event close is called always. Either the documentation should be changed or this PR should be reverted or fixed. I don't know if it is intended behaviour (then isn't it a API change - SEMVER-MAJOR?), but from my point of view only unintended side efect of the fix. It can break the functionality of many projects written according to the documentation (e.g. ours:). I can of course fix it by testing res.finished in close event...

@ronag
Copy link
Member Author

ronag commented Jun 28, 2018

@mareksrom this was fixed in 8ab7ea6

@mareksrom
Copy link

And how about SEMVER? Isn’t it a backward incompatible API change? It absolutely changes meaning of the close event... Is the documentation exact - how it is in keep alive mode when the connection is not closed with end of response?

@mareksrom
Copy link

@jasnell @trivirk Can I ask what was the reason for this change? I think it absolutely changes meaning of the close event. If there is a reason for this change it should be mentioned and should be SEMVER-MAJOR as a backward incompatible API change. Thanks

@tadjik1 tadjik1 mentioned this pull request Jul 5, 2018
Closed
2 tasks
@jasnell
Copy link
Member

jasnell commented Jul 11, 2018

Hey @mareksrom, my apologies as I just saw your notifications on this (I've been on vacation).

@nodejs/tsc @nodejs/streams ... in retrospect, this change likely should have been semver-major... It went out in 10.2 I believe. We could revert it in 10.x and keep in master for 11.x. Thoughts?

@mcollina
Copy link
Member

I agree with @jasnell. I think the correct behavior is what is current on master. The way it was behaving is against what the majority of users would expect on streams. 'close' should mean that the resource has been closed, and not represent a erroneous condition.

Regarding SEMVER, you are right @mareksrom. We should revert this in the 10.x line.

@targos targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 14, 2018
@targos targos mentioned this pull request Jul 14, 2018
Closed
2 tasks
@ggoodman ggoodman mentioned this pull request Jul 23, 2018
Merged
@targos targos mentioned this pull request Oct 24, 2018
Closed
@cjihrig cjihrig mentioned this pull request Jan 8, 2019
Closed
@lpinca lpinca mentioned this pull request Apr 30, 2019
Closed
michaltk added a commit to michaltk/bunyan-middleware that referenced this pull request Jun 27, 2019
@michaltk
Remove warning on res.close event
0474697 
nodejs/node#20611
tellnes#21
@wll8 wll8 mentioned this pull request Dec 11, 2021
Closed
@miniscruff miniscruff mentioned this pull request Jan 19, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OutgoingMessage doesn't always emit 'close'
10 participants
@ronag @mscdex @trivikr @mafintosh @watson @mareksrom @jasnell @mcollina @targos @nodejs-github-bot