[v10.x] Revert "http: always emit close on req and res" by targos · Pull Request #21809 · nodejs/node · GitHub

This also reverts 2a9c833 which modifies the added file and line.

Refs: #20611 (comment)

/cc @nodejs/tsc

The change landed in v10.2.0. I suppose there is a risk to break code that is now relying on it?

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

@targos build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/273/pipeline

I would prefer if this is a @nodejs/tsc decision. Should we put it in the agenda for next week?

Sure. Added the label.

The change landed in v10.2.0. I suppose there is a risk to break code that is now relying on it?

Node.js 10 has had the change for almost twice as long as it didn’t have it, and we’ve done security releases since then (i.e. we expect people to use versions that have it)… not knowing much about the subject matter, I’m not sure reverting is ideal?i

From different point of view - since 10.2.0 it was an undocumented API change, it became part of the documentation in version 10.6.0 just 10 days ago... How about projects relying on the previous documented version added in 0.6.7 released in January 2012? I think the number of them will be slighly bigger;)
I know it is not popular to do that, but SEMVER does not say could or should but MUST and we all know why (even though sometimes it doesn’t taste well:).

Note from TSC meeting: we also need to revert the documentation change.

From the discussion in the TSC meeting I'm +1 to reverting.

This is the documentation change #21047.

Was there a resolution in the TSC meeting? Should the tsc-agenda label be removed? Or is it slated for next week's meeting too? /cc @targos

@Trott we did not have quorum. The proposed path is to revert this in Node 10, but leave it in in master.

This approach would need necessarily 2 TSC signoffs because it's a retroactive semver-major change that should not have landed in master without 2 TSC signoffs.

This reverts commit 2a9c833.

This reverts commit 8029a24.

This reverts commit 8ab7ea6.

Doc change reverted

LGTM

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

Is this going to break end-of-stream again?

@targos I do not think so. What broke eos was 8029a24.
Moreover we are testing the same logic within stream.finished here. I'm confident that we are not breaking anything if CI passes.

cc @mafintosh to confirm before landing.

eos is fine with this. LGTM

CI is green. This needs at least one more @nodejs/tsc signoff.

Rubber stamp LGTM

@nodejs/release @nodejs/lts I think this is ready to land.

I'm going to add this to the release proposal at #22040

It's probably worth a sentence in the notable changes. Could someone help me write it?

Since this is already released, reverting it would just make a bad situation worse.

SEMVER was already broken once with the initial release, reverting it would just break it again.

I've added this back to the TSC agenda to discuss in today's meeting.

+1 reverting is the right move. This is something we should fix before LTS

LGTM

I feel the conflict here is which of the following we want to make migration from easier:

Earlier v10.x versions
Versions older than v10.x

Since v10.x is an LTS version, but we haven't reached LTS stage, there's still a lot of life remaining in v10.x as folks try to update to v10.x from versions older than v10.x.

It's a sucky situation, but I'm on the side of 👍 on this PR as well.

This reverts a commit that accidentally introduced a semver-major change to Node 10 and broke userland code. A subsequent fix to that change and documentation change are reverted with it. Revert "http: fix res emit close before user finish" This reverts commit 2a9c833. Revert "http: always emit close on req and res" This reverts commit 8029a24. Revert "doc: fix HTTP req/res 'close' description" This reverts commit 8ab7ea6. PR-URL: #21809 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Squashed and landed in 8799f43

targos added http Issues or PRs related to the http subsystem. v10.x labels Jul 14, 2018

nodejs-github-bot added http Issues or PRs related to the http subsystem. v10.x labels Jul 14, 2018

targos added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Jul 14, 2018

mhdawson mentioned this pull request Jul 16, 2018

Node.js Foundation Technical Steering Committee (TSC) Meeting 2018-07-18 nodejs/TSC#571

Closed

mcollina removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Jul 19, 2018

targos added 3 commits July 19, 2018 09:23

Revert "http: fix res emit close before user finish"

0b7537d

This reverts commit 2a9c833.

Revert "http: always emit close on req and res"

174d36d

This reverts commit 8029a24.

Revert "doc: fix HTTP req/res 'close' description"

7f6db65

This reverts commit 8ab7ea6.

targos force-pushed the revert-20611 branch from 7631312 to 7f6db65 Compare July 19, 2018 07:27

mcollina approved these changes Jul 19, 2018

View reviewed changes

mafintosh approved these changes Jul 19, 2018

View reviewed changes

MylesBorins approved these changes Jul 20, 2018

View reviewed changes

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 Jul 26, 2018

Node 10: http response.close is always triggered #21063

Closed

targos added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Aug 1, 2018

mhdawson mentioned this pull request Aug 1, 2018

Node.js Foundation Technical Steering Committee (TSC) Meeting 2018-08-01 nodejs/TSC#576

Closed

MylesBorins approved these changes Aug 1, 2018

View reviewed changes

rvagg approved these changes Aug 1, 2018

View reviewed changes

mhdawson approved these changes Aug 1, 2018

View reviewed changes

apapirovski approved these changes Aug 1, 2018

View reviewed changes

cjihrig approved these changes Aug 1, 2018

View reviewed changes

TimothyGu approved these changes Aug 1, 2018

View reviewed changes

targos removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Aug 1, 2018

targos closed this Aug 1, 2018

targos deleted the revert-20611 branch August 1, 2018 15:25

targos mentioned this pull request Aug 1, 2018

v10.8.0 proposal #22040

Merged

MylesBorins added the dont-land-on-v8.x label Aug 17, 2018

borisovg mentioned this pull request Dec 31, 2018

Node 10: close event is still emitted on normal HTTP response #25294

Closed

cjihrig mentioned this pull request Jan 8, 2019

Error tag incorrectly set (node 11) hapijs/hapi#3898

Closed

[v10.x] Revert "http: always emit close on req and res" by targos · Pull Request #21809 · nodejs/node · GitHub

Uh oh!

Uh oh!

[v10.x] Revert "http: always emit close on req and res" - #21809

[v10.x] Revert "http: always emit close on req and res"#21809targos wants to merge 3 commits intonodejs:v10.x-stagingnodejs/node:v10.x-stagingfrom targos:revert-20611targos/node:revert-20611Copy head branch name to clipboard

Conversation

targos commented Jul 14, 2018

Checklist

Uh oh!

nodejs-github-bot commented Jul 14, 2018

Uh oh!

mcollina commented Jul 14, 2018

Uh oh!

targos commented Jul 14, 2018

Uh oh!

addaleax commented Jul 14, 2018

Uh oh!

mareksrom commented Jul 15, 2018

Uh oh!

targos commented Jul 18, 2018

Uh oh!

mhdawson commented Jul 18, 2018

Uh oh!

mcollina commented Jul 18, 2018

Uh oh!

Trott commented Jul 18, 2018

Uh oh!

mcollina commented Jul 19, 2018

Uh oh!

targos commented Jul 19, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

targos commented Jul 19, 2018

Uh oh!

mcollina commented Jul 19, 2018

Uh oh!

mafintosh commented Jul 19, 2018

Uh oh!

targos commented Jul 20, 2018

Uh oh!

MylesBorins left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Jul 30, 2018

Uh oh!

targos commented Aug 1, 2018

Uh oh!

targos commented Aug 1, 2018

Uh oh!

ruiaraujo commented Aug 1, 2018

Uh oh!

targos commented Aug 1, 2018

Uh oh!

MylesBorins left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu left a comment • 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!

targos commented Aug 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

[v10.x] Revert "http: always emit close on req and res"#21809
targos wants to merge 3 commits into
nodejs:v10.x-stagingfrom
targos:revert-20611

TimothyGu left a comment •
edited
Loading