http: don't emit error after destroy #55457
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review requested:
|
This comment was marked as resolved.
This comment was marked as resolved.
nodejs-github-bot
added
http
ronag
force-pushed
the
no-error-after-destroy
branch
from
1dc7aba to
1fad5a9
Compare
ronag
force-pushed
the
no-error-after-destroy
branch
from
1fad5a9 to
67f5de3
Compare
github-actions
bot
removed
the
request-ci
anonrig
approved these changes
Oct 19, 2024
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #55457 +/- ##
==========================================
- Coverage 88.40% 88.39% -0.01%
==========================================
Files 652 653 +1
Lines 186777 187489 +712
Branches 36047 36095 +48
==========================================
+ Hits 165111 165740 +629
- Misses 14919 14980 +61
- Partials 6747 6769 +22
|
mcollina
added
the
baking-for-lts
mcollina
approved these changes
Oct 19, 2024
lpinca
approved these changes
Oct 19, 2024
Co-authored-by: Luigi Pinca <[email protected]>
github-actions
bot
removed
the
request-ci
Ethan-Arrowood
approved these changes
Oct 19, 2024
jasnell
approved these changes
Oct 20, 2024
|
I think it would good to have this fixed on LTS... It crashed and burned us in production. |
|
I don't remember how to start CITGM. Anyone care to help? |
ronag
added
the
author ready
|
Less failures than main so I guess this can land? |
ronag
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
Commit Queue failed- Loading data for nodejs/node/pull/55457 ✔ Done loading data for nodejs/node/pull/55457 ----------------------------------- PR info ------------------------------------ Title http: don't emit error after destroy (#55457) Author Robert Nagy <[email protected]> (@ronag) Branch ronag:no-error-after-destroy -> nodejs:main Labels http, baking-for-lts, author ready, needs-ci, needs-citgm Commits 2 - http: don't emit error after destroy - Update test-http-outgoing-destroyed.js Committers 2 - Robert Nagy <[email protected]> - GitHub <[email protected]> PR-URL: https://github.com/nodejs/node/pull/55457 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: James M Snell <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/55457 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: James M Snell <[email protected]> -------------------------------------------------------------------------------- ℹ This PR was created on Sat, 19 Oct 2024 09:16:44 GMT ✔ Approvals: 5 ✔ - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/55457#pullrequestreview-2379567206 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/55457#pullrequestreview-2379652399 ✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/55457#pullrequestreview-2379661418 ✔ - Ethan Arrowood (@Ethan-Arrowood): https://github.com/nodejs/node/pull/55457#pullrequestreview-2379742837 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/55457#pullrequestreview-2380690848 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-10-19T19:28:30Z: https://ci.nodejs.org/job/node-test-pull-request/63203/ ℹ Last CITGM CI on 2024-10-21T05:34:31Z: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3492/ - Querying data for job/node-test-pull-request/3492/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 55457 From https://github.com/nodejs/node * branch refs/pull/55457/merge -> FETCH_HEAD ✔ Fetched commits as 7ddd2c228296..ddcfee78255b -------------------------------------------------------------------------------- [main b05441fc9e] http: don't emit error after destroy Author: Robert Nagy <[email protected]> Date: Sat Oct 19 11:16:00 2024 +0200 2 files changed, 27 insertions(+), 1 deletion(-) [main 328ad3e913] Update test-http-outgoing-destroyed.js Author: Robert Nagy <[email protected]> Date: Sat Oct 19 20:33:05 2024 +0200 1 file changed, 1 insertion(+), 1 deletion(-) ✔ Patches applied There are 2 commits in the PR. Attempting autorebase. Rebasing (2/4) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- http: don't emit error after destroyhttps://github.com/nodejs/node/actions/runs/11515733656 |
nodejs-github-bot
added
the
commit-queue-failed
ronag
added
commit-queue-squash
jakecastelli
approved these changes
Oct 28, 2024
ronag
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 5633c62 |
Ceres6
pushed a commit
to Ceres6/node
that referenced
this pull request
Oct 30, 2024
PR-URL: nodejs#55457 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jake Yuesong Li <[email protected]>
RafaelGSS
pushed a commit
that referenced
this pull request
Nov 1, 2024
PR-URL: #55457 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jake Yuesong Li <[email protected]>
louwers
pushed a commit
to louwers/node
that referenced
this pull request
Nov 2, 2024
PR-URL: nodejs#55457 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jake Yuesong Li <[email protected]>
lukiano
reviewed
Nov 13, 2024
| @@ -908,6 +908,10 @@ OutgoingMessage.prototype.write = function write(chunk, encoding, callback) { | |||
| }; | |||
|
|
|||
| function onError(msg, err, callback) { | |||
| if (msg.destroyed) { | |||
| return; | |||
There was a problem hiding this comment.
Does this mean that if I call end(chunk, CB) and this is both finished and destroyed, the callback won't be called?
tpoisseau
pushed a commit
to tpoisseau/node
that referenced
this pull request
Nov 21, 2024
PR-URL: nodejs#55457 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jake Yuesong Li <[email protected]>
aduh95
pushed a commit
to aduh95/node
that referenced
this pull request
Dec 4, 2024
PR-URL: nodejs#55457 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jake Yuesong Li <[email protected]>
aduh95
pushed a commit
that referenced
this pull request
Dec 10, 2024
PR-URL: #55457 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jake Yuesong Li <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Enforce stream invariant. No new errors can occur after
.destroy(err)