node http: don't emit error after destroy

Enforce stream invariant. No new errors can occur after .destroy(err)

Oct 19 '24 09:10 ronag

Review requested:

[x] @nodejs/http
[ ] @nodejs/net

Oct 19 '24 09:10 nodejs-github-bot

test WIP

Oct 19 '24 09:10 ronag

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

Oct 19 '24 14:10 nodejs-github-bot

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.39%. Comparing base (d881fcb) to head (ddcfee7). Report is 108 commits behind head on main.

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

Files with missing lines	Coverage Δ
lib/_http_outgoing.js	`94.99% <100.00%> (-0.07%)`	:arrow_down:

... and 132 files with indirect coverage changes

Oct 19 '24 15:10 codecov[bot]

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

Oct 19 '24 19:10 nodejs-github-bot

I think it would good to have this fixed on LTS... It crashed and burned us in production.

Oct 21 '24 04:10 ronag

I don't remember how to start CITGM. Anyone care to help?

Oct 21 '24 05:10 ronag

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3490/ CITGM (main): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3492/

Oct 21 '24 05:10 lpinca

Less failures than main so I guess this can land?

Oct 25 '24 09:10 ronag

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 destroy
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]>
[detached HEAD 91ad305189] 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(-)
Rebasing (3/4)
Rebasing (4/4)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
Update test-http-outgoing-destroyed.js
Co-authored-by: Luigi Pinca <[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]>
[detached HEAD 0f2e9b3c39] 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(-)
Successfully rebased and updated refs/heads/main.
ℹ  Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/11515733656

Oct 25 '24 09:10 nodejs-github-bot

Landed in 5633c62219e199baac833e8862d60333d85dc3d3

Oct 28 '24 12:10 nodejs-github-bot

http: don't emit error after destroy

Codecov Report

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

[detached HEAD 0f2e9b3c39] 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(-) Successfully rebased and updated refs/heads/main.