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

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

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Jul 14, 2018

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 targos added http Issues or PRs related to the http subsystem. v10.x labels Jul 14, 2018
@nodejs-github-bot
Copy link
Collaborator

@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

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

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

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

targos commented Jul 14, 2018

Sure. Added the label.

@addaleax
Copy link
Member

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

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

@mhdawson mhdawson mentioned this pull request Jul 16, 2018
Closed
@targos
Copy link
Member Author

targos commented Jul 18, 2018

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

@mhdawson
Copy link
Member

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

@mcollina
Copy link
Member

This is the documentation change #21047.

@Trott
Copy link
Member

Trott commented Jul 18, 2018

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 mcollina removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Jul 19, 2018
@mcollina
Copy link
Member

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

@targos
Copy link
Member Author

targos commented Jul 19, 2018

Doc change reverted

mcollina
mcollina approved these changes Jul 19, 2018
View reviewed changes
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

@targos
Copy link
Member Author

targos commented Jul 19, 2018

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

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

@mcollina
Copy link
Member

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

eos is fine with this. LGTM

@targos
Copy link
Member Author

targos commented Jul 20, 2018

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

MylesBorins
MylesBorins approved these changes Jul 20, 2018
View reviewed changes
Copy link
Member

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

Rubber stamp LGTM

@ggoodman ggoodman mentioned this pull request Jul 23, 2018
Merged
@mcollina
Copy link
Member

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

@targos
Copy link
Member Author

targos commented Aug 1, 2018

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

@targos
Copy link
Member Author

targos commented Aug 1, 2018

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

@ruiaraujo
Copy link

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 targos added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Aug 1, 2018
@targos
Copy link
Member Author

targos commented Aug 1, 2018

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

@mhdawson mhdawson mentioned this pull request Aug 1, 2018
Closed
MylesBorins
MylesBorins approved these changes Aug 1, 2018
View reviewed changes
Copy link
Member

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

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

mhdawson
mhdawson approved these changes Aug 1, 2018
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

TimothyGu
TimothyGu approved these changes Aug 1, 2018
View reviewed changes
Copy link
Member

@TimothyGu TimothyGu left a comment •
edited

Choose a reason for hiding this comment

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

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.

@targos targos removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Aug 1, 2018
targos added a commit that referenced this pull request Aug 1, 2018
@targos
http: revert "http: always emit close on req and res"
8799f43 
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
Copy link
Member Author

targos commented Aug 1, 2018

Squashed and landed in 8799f43

@targos targos closed this Aug 1, 2018
@targos targos deleted the revert-20611 branch August 1, 2018 15:25
@targos targos mentioned this pull request Aug 1, 2018
Merged
This was referenced Aug 1, 2018
Closed
Closed
Closed
Closed
Closed
Closed
@targos targos added this to Backports done in v10.x Sep 23, 2018
@borisovg borisovg mentioned this pull request Dec 31, 2018
Closed
@cjihrig cjihrig mentioned this pull request Jan 8, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apapirovski apapirovski apapirovski approved these changes

@mcollina mcollina mcollina approved these changes

@mafintosh mafintosh mafintosh approved these changes

@rvagg rvagg rvagg approved these changes

@MylesBorins MylesBorins MylesBorins approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@cjihrig cjihrig cjihrig approved these changes

@mhdawson mhdawson mhdawson approved these changes

Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem.
Projects
No open projects
v10.x
  
Closed PRs
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet