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

targos · 2018-07-14T10:57:05Z

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

nodejs-github-bot · 2018-07-14T10:57:06Z

@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

mcollina · 2018-07-14T11:13:48Z

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

targos · 2018-07-14T11:15:38Z

Sure. Added the label.

addaleax · 2018-07-14T23:29:25Z

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

mareksrom · 2018-07-15T10:42:48Z

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:).

targos · 2018-07-18T17:17:03Z

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

mhdawson · 2018-07-18T17:20:48Z

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

mcollina · 2018-07-18T17:21:17Z

This is the documentation change #21047.

Trott · 2018-07-18T23:44:59Z

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

mcollina · 2018-07-19T07:15:55Z

@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.

targos · 2018-07-19T07:27:48Z

Doc change reverted

mcollina

LGTM

targos · 2018-07-19T09:07:22Z

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

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

mcollina · 2018-07-19T09:36:40Z

@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.

mafintosh · 2018-07-19T12:19:04Z

eos is fine with this. LGTM

targos · 2018-07-20T16:28:28Z

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

MylesBorins

Rubber stamp LGTM

mcollina · 2018-07-30T15:00:20Z

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

targos · 2018-08-01T08:00:45Z

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

targos · 2018-08-01T08:01:44Z

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

ruiaraujo · 2018-08-01T09:43:28Z

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.

targos · 2018-08-01T12:18:56Z

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

MylesBorins

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

mhdawson

LGTM

TimothyGu

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>

targos · 2018-08-01T15:25:25Z

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 7f6db65Compare 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

Uh oh!

Uh oh!

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

TimothyGu left a comment •
edited
Loading