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.

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

Revert "http: do not leak error listeners" #44921

Merged
merged 1 commit into from
Oct 12, 2022

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Oct 8, 2022

This reverts commit 736a7d8.

The patch attempted to fix an issue, signaled by a warning, caused by an invalid API usage. However it introduced a behavior change that is breaking userland modules.

It is a user error, so restore the original behavior and have the user investigate and fix the issue in their code.

Refs: #43587 (comment)
Refs: #43587 (comment)

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Oct 8, 2022
This reverts commit 736a7d8.

The patch attempted to fix an issue, signaled by a warning, caused by
an invalid API usage. However it introduced a behavior change that is
breaking userland modules.

It is a user error, so restore the original behavior and have the user
investigate and fix the issue in their code.

Refs: nodejs#43587 (comment)
Refs: nodejs#43587 (comment)
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

@mcollina
Copy link
Member

mcollina commented Oct 8, 2022

It might be better to add a test that document this.

@lpinca
Copy link
Member Author

lpinca commented Oct 8, 2022

It might be better to add a test that document this.

Ok, but in a follow up PR, possibly. This is only a revert.

@lpinca lpinca mentioned this pull request Oct 11, 2022
@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 11, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 11, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/44921
✔  Done loading data for nodejs/node/pull/44921
----------------------------------- PR info ------------------------------------
Title      Revert "http: do not leak error listeners" (#44921)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     lpinca:revert/736a7d8 -> nodejs:main
Labels     http, needs-ci
Commits    1
 - Revert "http: do not leak error listeners"
Committers 1
 - Luigi Pinca 
PR-URL: https://github.com/nodejs/node/pull/44921
Reviewed-By: Matteo Collina 
Reviewed-By: Robert Nagy 
Reviewed-By: Colin Ihrig 
Reviewed-By: Beth Griggs 
Reviewed-By: Tierney Cyren 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/44921
Reviewed-By: Matteo Collina 
Reviewed-By: Robert Nagy 
Reviewed-By: Colin Ihrig 
Reviewed-By: Beth Griggs 
Reviewed-By: Tierney Cyren 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 08 Oct 2022 05:49:52 GMT
   ✔  Approvals: 5
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/44921#pullrequestreview-1135185005
   ✔  - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/44921#pullrequestreview-1135192504
   ✔  - Colin Ihrig (@cjihrig) (TSC): https://github.com/nodejs/node/pull/44921#pullrequestreview-1135251333
   ✔  - Beth Griggs (@BethGriggs) (TSC): https://github.com/nodejs/node/pull/44921#pullrequestreview-1135400540
   ✔  - Tierney Cyren (@bnb): https://github.com/nodejs/node/pull/44921#pullrequestreview-1136569280
   ✔  Last GitHub CI successful
   ✖  No Jenkins CI runs detected
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/3229727303

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 11, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 11, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 12, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 12, 2022
@nodejs-github-bot nodejs-github-bot merged commit 7c06eab into nodejs:main Oct 12, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 7c06eab

danielleadams pushed a commit that referenced this pull request Dec 30, 2022
This reverts commit 736a7d8.

The patch attempted to fix an issue, signaled by a warning, caused by
an invalid API usage. However it introduced a behavior change that is
breaking userland modules.

It is a user error, so restore the original behavior and have the user
investigate and fix the issue in their code.

Refs: #43587 (comment)
Refs: #43587 (comment)
PR-URL: #44921
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
This reverts commit 736a7d8.

The patch attempted to fix an issue, signaled by a warning, caused by
an invalid API usage. However it introduced a behavior change that is
breaking userland modules.

It is a user error, so restore the original behavior and have the user
investigate and fix the issue in their code.

Refs: #43587 (comment)
Refs: #43587 (comment)
PR-URL: #44921
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-failed An error occurred while landing this pull request using GitHub Actions. http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants