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: remove default 'error' listener on upgrade #18868

Closed
wants to merge 1 commit into from
Closed

http: remove default 'error' listener on upgrade #18868

wants to merge 1 commit into from

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Feb 19, 2018

Remove the default 'error' listener when the socket is freed. This is consistent with the client and prevents spurious 'clientError' events from being emitted on the server.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

http

@lpinca
http: remove default 'error' listener on upgrade
6f3a177 
Remove the default `'error'` listener when the socket is freed. This
is consistent with the client and prevents spurious `'clientError'`
events from being emitted on the server.
@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Feb 19, 2018
@lpinca lpinca added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 19, 2018
@lpinca
Copy link
Member Author

lpinca commented Feb 19, 2018

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

@BridgeAR BridgeAR added fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 20, 2018
@lpinca lpinca removed fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 20, 2018
@lpinca
Copy link
Member Author

lpinca commented Feb 20, 2018 •
edited
Loading

Removed some labels as this needs at least 2 TSC approvals.

@BridgeAR
Copy link
Member

BridgeAR commented Feb 20, 2018 •
edited
Loading

@lpinca just as a thing about how I understand the ready label: I always add it as soon as it gets approved and there are no concerns and the code change looks ready and the CI was run. It does not mean for me, that there are enough LGs.

About fast tracking - somewhat the same. It needs the two LGs and I wait for others to "approve" the fast-track.

Therefore I still think the two labels apply. Besides the fact that I personally do not think that this is semver-major. I see this as a patch.

@BridgeAR BridgeAR requested a review from a team February 20, 2018 16:46
@lpinca
Copy link
Member Author

lpinca commented Feb 20, 2018 •
edited
Loading

For me "ready" means ready to be landed and "fast track" a patch that doesn't need to wait the 48h or a critical bug fix.

It is definitely semver-major. It's currently possible to not add an error listener after the upgrade if you are fine with the default one. With this patch this is no longer true.

@BridgeAR
Copy link
Member

@lpinca we do not really diverge in the meaning here. fast-track is clearly about not having to wait that long and I made a mistake because semver-major things should not be fast-tracked. Sorry about that. I was just looking briefly into it.

About the ready part: I think it makes sense to stick the label on it as soon as the CI is run and it got at least someone agreeing, even if it should still stay there a bit longer. Because if it should not stay there for at least a bit longer it should instead be landed right away, right?

@lpinca
Copy link
Member Author

lpinca commented Feb 20, 2018 •
edited
Loading

About the ready part: I think it makes sense to stick the label on it as soon as the CI is run and it got at least someone agreeing, even if it should still stay there a bit longer.

I think this goes against the 48h rule. If 48h did not pass it's not fully backed, thus not ready in my opinion.

That said if it's ok to put the ready label as soon as there are enough approvals and a good CI so it be.

Edit: maybe the ready label was introduced to signal that a PR has enough approvals and a good CI. In that case ignore me as I 100% agree that it should be added as soon as possible.

@lpinca
Copy link
Member Author

lpinca commented Feb 21, 2018

@BridgeAR it looks like your usage of the "ready" label is correct as per discussion in #17124, so I'm adding it back now that there are 2 TSC approvals.

@lpinca lpinca added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 21, 2018
@BridgeAR
Copy link
Member

Landed in 51be03c 🎉

@BridgeAR BridgeAR closed this Feb 22, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 22, 2018
@lpinca @BridgeAR
http: remove default 'error' listener on upgrade
51be03c 
Remove the default `'error'` listener when the socket is freed. This
is consistent with the client and prevents spurious `'clientError'`
events from being emitted on the server.

PR-URL: nodejs#18868
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@lpinca lpinca deleted the remove/error-listener branch February 22, 2018 16:18
@lpinca lpinca mentioned this pull request Mar 7, 2018
Closed
4 tasks
@lpinca lpinca mentioned this pull request May 2, 2018
Closed
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@lpinca @MayaLekova
http: remove default 'error' listener on upgrade
2097733 
Remove the default `'error'` listener when the socket is freed. This
is consistent with the client and prevents spurious `'clientError'`
events from being emitted on the server.

PR-URL: nodejs#18868
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@addaleax addaleax mentioned this pull request May 2, 2019
Open
@lpinca lpinca mentioned this pull request May 25, 2019
Closed
@rgoulter rgoulter mentioned this pull request Jun 24, 2019
Closed
@briannielson briannielson mentioned this pull request Jun 9, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@benjamingr benjamingr benjamingr approved these changes

@daynin daynin daynin approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.
7 participants
@lpinca @BridgeAR @jasnell @daynin @benjamingr @joyeecheung @nodejs-github-bot