http: always emit close on req and res by ronag · Pull Request #20611 · nodejs/node

ronag · 2018-05-08T19:57:43Z

'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

mscdex · 2018-05-08T21:06:35Z

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

mscdex · 2018-05-09T17:34:52Z

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

trivikr · 2018-05-13T20:20:53Z

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

PR-URL: #20611Fixes: #20600 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

trivikr · 2018-05-13T23:24:46Z

Landed in f22c7c1

PR-URL: #20611Fixes: #20600 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

Changes introduced in: nodejs/node#20941 nodejs/node#20611

mareksrom · 2018-06-27T20:13:01Z

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 · 2018-06-28T11:05:17Z

@mareksrom this was fixed in 8ab7ea6

mareksrom · 2018-06-28T12:42:48Z

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 · 2018-06-29T07:08:29Z

@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

jasnell · 2018-07-11T13:53:06Z

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 · 2018-07-12T17:40:02Z

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.

nodejs/node#20611 tellnes#21

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

ronag changed the title ~~Fix emit close req res~~http: fix emit close req resMay 8, 2018

ronag force-pushed the fix-emit-close-req-res branch 4 times, most recently from f389c78 to 8ce8826Compare May 8, 2018 20:00

ronag changed the title ~~http: fix emit close req res~~http: always emit close on req and resMay 8, 2018

http: always emit close on req and res
30896bc

ronag force-pushed the fix-emit-close-req-res branch from 8ce8826 to 30896bcCompare May 9, 2018 09:15

jasnell approved these changes May 9, 2018
View reviewed changes

trivikr approved these changes May 9, 2018
View reviewed changes

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

trivikr closed this May 13, 2018

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

addaleax mentioned this pull request May 14, 2018
v10.2.0 proposal #20724
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 mentioned this pull request Jun 6, 2018
fix(server): address Node v10.x req close event firing order restify/node-restify#1672
Merged

tadjik1 mentioned this pull request Jul 5, 2018
doc: fix HTTP res 'finish' description #21670
Closed
2 tasks

targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 14, 2018

targos mentioned this pull request Jul 14, 2018
[v10.x] Revert "http: always emit close on req and res" #21809
Closed
2 tasks

ggoodman mentioned this pull request Jul 23, 2018
Defer piping of options.payload until socket connection hapijs/wreck#223
Merged

targos mentioned this pull request Oct 24, 2018
Missing semver-major change in v11.0.0 changelog #23846
Closed

cjihrig mentioned this pull request Jan 8, 2019
Error tag incorrectly set (node 11) hapijs/hapi#3898
Closed

lpinca mentioned this pull request Apr 30, 2019
http: missing close after finish #27480
Closed

michaltk added a commit to michaltk/bunyan-middleware that referenced this pull request Jun 27, 2019
Remove warning on res.close event
0474697
nodejs/node#20611 tellnes#21

wll8 mentioned this pull request Dec 11, 2021
fix: 某些情况没有保存请求记录到 json 文件中 wll8/mockm#26
Closed

miniscruff mentioned this pull request Jan 19, 2022
INTEGRATION [PR#1649 > development/8.1] ARSN-21 Upgrade Node to 16 scality/Arsenal#1663
Merged

Navigation Menu

Uh oh!

Uh oh!

http: always emit close on req and res - #20611

http: always emit close on req and res#20611
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:fix-emit-close-req-res

ronag commented May 8, 2018•
edited
Loading

Uh oh!

mscdex commented May 8, 2018

Uh oh!

mscdex commented May 9, 2018

Uh oh!

trivikr commented May 13, 2018

Uh oh!

trivikr commented May 13, 2018

Uh oh!

mareksrom commented Jun 27, 2018

Uh oh!

ronag commented Jun 28, 2018

Uh oh!

mareksrom commented Jun 28, 2018

Uh oh!

mareksrom commented Jun 29, 2018

Uh oh!

jasnell commented Jul 11, 2018

Uh oh!

mcollina commented Jul 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Uh oh!

Uh oh!

Conversation

ronag commented May 8, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

mscdex commented May 8, 2018

Uh oh!

mscdex commented May 9, 2018

Uh oh!

trivikr commented May 13, 2018

Uh oh!

trivikr commented May 13, 2018

Uh oh!

mareksrom commented Jun 27, 2018

Uh oh!

ronag commented Jun 28, 2018

Uh oh!

mareksrom commented Jun 28, 2018

Uh oh!

mareksrom commented Jun 29, 2018

Uh oh!

jasnell commented Jul 11, 2018

Uh oh!

mcollina commented Jul 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

ronag commented May 8, 2018•
edited
Loading