Skip to content

http: don't emit error for stream destroyed#33654

Closed
ronag wants to merge 3 commits intonodejs:masterfrom
nxtedition:outgoing-error-close
Closed

http: don't emit error for stream destroyed#33654
ronag wants to merge 3 commits intonodejs:masterfrom
nxtedition:outgoing-error-close

Conversation

@ronag
Copy link
Member

@ronag ronag commented May 30, 2020 •
edited
Loading

Aligns with streams

Refs: #33591

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

@ronag ronag added http Issues or PRs related to the http subsystem. v14.x labels May 30, 2020
@ronag ronag requested a review from addaleax May 30, 2020 08:41
@ronag ronag force-pushed the outgoing-error-close branch from 72c7eac to 734bf3a Compare May 30, 2020 08:42
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 31, 2020

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

@ronag
Copy link
Member Author

ronag commented May 31, 2020

@nodejs/http @nodejs/web-server-frameworks

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 31, 2020

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

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
@ronag ronag changed the title http: don't emit error after close http: don't emit error for stream destroyed May 31, 2020
@ronag ronag force-pushed the outgoing-error-close branch 2 times, most recently from b152c48 to a21385e Compare May 31, 2020 19:14
@ronag
Copy link
Member Author

ronag commented May 31, 2020

unsure about the semversiness of this one.

@ronag ronag force-pushed the outgoing-error-close branch from a21385e to 48a58b1 Compare May 31, 2020 19:45
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 31, 2020

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

ronag
ronag commented Jun 1, 2020
View reviewed changes
@ronag ronag force-pushed the outgoing-error-close branch 2 times, most recently from 92d8ad6 to 73b6a74 Compare June 1, 2020 15:38
mcollina
mcollina reviewed Jun 1, 2020
View reviewed changes
@ronag ronag mentioned this pull request Jun 1, 2020
Closed
4 tasks
This was referenced Jun 4, 2020
Closed
Open
Open
Open
Open
Open
@BethGriggs
Copy link
Member

BethGriggs commented Jun 9, 2020

Should this land in the next v14.x release?

@ronag ronag requested a review from mcollina June 9, 2020 15:30
mcollina
mcollina approved these changes Jun 9, 2020
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, crossing fingers we do not break anything.

@ronag ronag force-pushed the outgoing-error-close branch from 0d71e89 to 7191d18 Compare June 15, 2020 20:56
@ronag
Copy link
Member Author

ronag commented Jun 15, 2020

@mcollina: I've updated the test so that it doesn't have to be commented out. I don't think #33684 should be blocking this PR.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 15, 2020

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

mcollina
mcollina approved these changes Jun 16, 2020
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

This was referenced Jun 18, 2020
Open
Open
Open
@ronag ronag force-pushed the outgoing-error-close branch from 7191d18 to 402fc26 Compare June 20, 2020 11:58
@ronag
Copy link
Member Author

ronag commented Jun 20, 2020

This needs another CI run + CITGM

@addaleax addaleax added needs-ci PRs that need a full CI run. needs-citgm PRs that need a CITGM CI run. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed v14.x needs-ci PRs that need a full CI run. needs-citgm PRs that need a CITGM CI run. labels Jun 20, 2020
@addaleax
Copy link
Member

addaleax commented Jun 20, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/31991/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2394/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 21, 2020 •
edited by ronag
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/32001/ ✅
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2395/
CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2396/

@ronag
Copy link
Member Author

ronag commented Jun 21, 2020

Landed in 30cc542

@ronag ronag closed this Jun 21, 2020
ronag added a commit that referenced this pull request Jun 21, 2020
@ronag
http: don't emit error after close
30cc542 
Refs: #33591

PR-URL: #33654
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@MylesBorins
Copy link
Contributor

MylesBorins commented Jul 14, 2020

This doesn't land cleanly on v14.x. @ronag would you be willing to backport?

@ronag ronag self-assigned this Jul 14, 2020
@Lxxyx Lxxyx mentioned this pull request Jan 5, 2021
Closed
3 tasks
@targos
Copy link
Member

targos commented May 16, 2021

Should this be backported with #32933 ?

@ronag
Copy link
Member Author

ronag commented May 16, 2021

Yes. I'm a little overloaded at the moment though. If you need help ping me again in a. week :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax Awaiting requested review from addaleax

Assignees

@ronag ronag

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@ronag @nodejs-github-bot @BethGriggs @addaleax @MylesBorins @targos @mcollina @jasnell