Skip to content

http2: send RST code 8 on AbortController signal - #48573

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
devm33:devm33/http2-abort
Jul 6, 2023
Merged

http2: send RST code 8 on AbortController signal#48573
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
devm33:devm33/http2-abort

Conversation

@devm33

@devm33devm33 commented Jun 27, 2023

Copy link
Copy Markdown
Contributor

Fixes: #47321

@nodejs-github-bot

nodejs-github-bot commented Jun 27, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-botnodejs-github-bot added http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Jun 27, 2023
@devm33devm33 mentioned this pull request Jun 27, 2023
Closed
@devm33
devm33force-pushed the devm33/http2-abort branch from cc9c457 to 7f88039CompareJune 27, 2023 15:20
@devm33

devm33 commented Jun 27, 2023

Copy link
Copy Markdown
ContributorAuthor

@anonrig Updated commit message!

@RafaelGSSRafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 27, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 27, 2023
@nodejs-github-bot

nodejs-github-bot commented Jun 27, 2023

Copy link
Copy Markdown
Collaborator

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

RafaelGSS
RafaelGSS approved these changes Jun 27, 2023
View reviewed changes

@RafaelGSSRafaelGSS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with green CI

@github-actionsgithub-actionsBot mentioned this pull request Jun 28, 2023
Open
36 tasks
@devm33

devm33 commented Jun 28, 2023

Copy link
Copy Markdown
ContributorAuthor

Thanks @RafaelGSS! It looks to me that the failing tests are unrelated. Is it possible they are flakes and if so can they be rerun?

@nodejs-github-bot

nodejs-github-bot commented Jun 28, 2023

Copy link
Copy Markdown
Collaborator

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

@devm33

devm33 commented Jun 28, 2023

Copy link
Copy Markdown
ContributorAuthor

Thanks @RafaelGSS! It looks to me that the failing tests are unrelated. Is it possible they are flakes and if so can they be rerun?

Looks like a different test flake this time 🙃 Let me know if there are any tips for getting the CI green!

@RafaelGSS

RafaelGSS commented Jun 28, 2023

Copy link
Copy Markdown
Member

Thanks @RafaelGSS! It looks to me that the failing tests are unrelated. Is it possible they are flakes and if so can they be rerun?

Looks like a different test flake this time upside_down_face Let me know if there are any tips for getting the CI green!

For now, we can Just retry

@nodejs-github-bot

nodejs-github-bot commented Jun 28, 2023

Copy link
Copy Markdown
Collaborator

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

@devm33

devm33 commented Jun 28, 2023

Copy link
Copy Markdown
ContributorAuthor

For now, we can Just retry

Gotcha, in that case thank you @RafaelGSS for retrying! 🔁

This was referenced Jun 29, 2023
Open
Open
@nodejs-github-bot

nodejs-github-bot commented Jun 30, 2023

Copy link
Copy Markdown
Collaborator

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

@github-actionsgithub-actionsBot mentioned this pull request Jul 1, 2023
Open
30 tasks
@devm33

devm33 commented Jul 1, 2023

Copy link
Copy Markdown
ContributorAuthor

For now, we can Just retry

Gotcha, in that case thank you @RafaelGSS for retrying! 🔁

Thanks for continuing to rerun @RafaelGSS! Exciting to see it all green 🟢!

@github-actionsgithub-actionsBot mentioned this pull request Jul 2, 2023
Open
27 tasks
@RafaelGSSRafaelGSS added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 2, 2023
This was referenced Jul 3, 2023
Open
Open
Open
@github-actionsgithub-actionsBot mentioned this pull request Jul 6, 2023
Open
17 tasks
@RafaelGSSRafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 6, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 6, 2023
@nodejs-github-bot
nodejs-github-bot merged commit b5e16ad into nodejs:mainJul 6, 2023
@nodejs-github-bot

nodejs-github-bot commented Jul 6, 2023

Copy link
Copy Markdown
Collaborator

Landed in b5e16ad

@devm33
devm33 deleted the devm33/http2-abort branch July 7, 2023 14:40
@devm33

devm33 commented Jul 7, 2023

Copy link
Copy Markdown
ContributorAuthor

Hi @richardlau do I need to do anything to backport this to v18? It looks like it merges cleanly onto the v18.x-staging branch.

Also could this be considered for v16? It looks like the commit applies cleanly to the v16.x-staging branch as well.

@RafaelGSS

RafaelGSS commented Jul 7, 2023

Copy link
Copy Markdown
Member

v16.x is under maintainer mode. We don't perform regular releases.

If this land cleanly on v18.x, it should be included in the next release (v18.18.0).

@richardlau

richardlau commented Jul 7, 2023

Copy link
Copy Markdown
Member

@devm33 This would normally need to go out in a Node.js current release (i.e. 20.x) first before it would be eligible for inclusion in an LTS release.

It would not normally be considered for Node.js 16 now that is in maintenance.

@devm33

devm33 commented Jul 7, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Thanks @RafaelGSS and @richardlau! That makes sense. Would be nice to consider as a bug fix for 16, but definitely understand if it doesn't make it in.

juanarbol pushed a commit that referenced this pull request Jul 13, 2023
@devm33@juanarbol
http2: send RST code 8 on AbortController signal
4a0b66e
Fixes: #47321
Refs: https://www.rfc-editor.org/rfc/rfc7540#section-7
PR-URL: #48573
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@juanarboljuanarbol mentioned this pull request Jul 13, 2023
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@devm33@Ceres6
http2: send RST code 8 on AbortController signal
453e7d3
Fixes: nodejs#47321
Refs: https://www.rfc-editor.org/rfc/rfc7540#section-7
PR-URL: nodejs#48573
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@devm33@Ceres6
http2: send RST code 8 on AbortController signal
21c6bc4
Fixes: nodejs#47321
Refs: https://www.rfc-editor.org/rfc/rfc7540#section-7
PR-URL: nodejs#48573
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
ruyadorno pushed a commit that referenced this pull request Sep 11, 2023
@devm33@ruyadorno
http2: send RST code 8 on AbortController signal
1c591b1
Fixes: #47321
Refs: https://www.rfc-editor.org/rfc/rfc7540#section-7
PR-URL: #48573
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@ruyadornoruyadorno mentioned this pull request Sep 11, 2023
Merged
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
@devm33@ruyadorno
http2: send RST code 8 on AbortController signal
71da5dc
Fixes: #47321
Refs: https://www.rfc-editor.org/rfc/rfc7540#section-7
PR-URL: #48573
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
ruyadorno pushed a commit that referenced this pull request Sep 17, 2023
@devm33@ruyadorno
http2: send RST code 8 on AbortController signal
7c7230a
Fixes: #47321
Refs: https://www.rfc-editor.org/rfc/rfc7540#section-7
PR-URL: #48573
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonriganonriganonrig approved these changes
@JungMinuJungMinuJungMinu approved these changes
@RafaelGSSRafaelGSSRafaelGSS approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.http2Issues or PRs related to the http2 subsystem.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

HTTP/2 streams cancelled with an AbortSignal should close with NGHTTP2_CANCEL instead of NGHTTP2_INTERNAL_ERROR

7 participants

@devm33@nodejs-github-bot@RafaelGSS@richardlau@anonrig@JungMinu@targos