Skip to content

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

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
Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@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