Skip to content

http2: send RST code 8 on AbortController signal#48573

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

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

Conversation

@devm33
Copy link
Contributor

@devm33 devm33 commented Jun 27, 2023

Fixes: #47321

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 27, 2023

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot nodejs-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
@devm33 devm33 mentioned this pull request Jun 27, 2023
Closed
@devm33 devm33 force-pushed the devm33/http2-abort branch from cc9c457 to 7f88039 Compare June 27, 2023 15:20
@devm33
Copy link
Contributor Author

devm33 commented Jun 27, 2023

@anonrig Updated commit message!

@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 27, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 27, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 27, 2023

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

RafaelGSS
RafaelGSS approved these changes Jun 27, 2023
View reviewed changes
Copy link
Member

@RafaelGSS RafaelGSS left a comment

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-actions github-actions bot mentioned this pull request Jun 28, 2023
Open
36 tasks
@devm33
Copy link
Contributor Author

devm33 commented Jun 28, 2023

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
Copy link
Collaborator

nodejs-github-bot commented Jun 28, 2023

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

@devm33
Copy link
Contributor Author

devm33 commented Jun 28, 2023

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
Copy link
Member

RafaelGSS commented Jun 28, 2023

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
Copy link
Collaborator

nodejs-github-bot commented Jun 28, 2023

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

@devm33
Copy link
Contributor Author

devm33 commented Jun 28, 2023

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
Copy link
Collaborator

nodejs-github-bot commented Jun 30, 2023

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

@github-actions github-actions bot mentioned this pull request Jul 1, 2023
Open
30 tasks
@devm33
Copy link
Contributor Author

devm33 commented Jul 1, 2023

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-actions github-actions bot mentioned this pull request Jul 2, 2023
Open
27 tasks
@RafaelGSS RafaelGSS 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-actions github-actions bot mentioned this pull request Jul 6, 2023
Open
17 tasks
@RafaelGSS RafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 6, 2023
@nodejs-github-bot nodejs-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:main Jul 6, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 6, 2023

Landed in b5e16ad

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

devm33 commented Jul 7, 2023

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
Copy link
Member

RafaelGSS commented Jul 7, 2023

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
Copy link
Member

richardlau commented Jul 7, 2023

@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
Copy link
Contributor Author

devm33 commented Jul 7, 2023 •
edited
Loading

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>
@juanarbol juanarbol 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>
@ruyadorno ruyadorno 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 free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonrig anonrig anonrig approved these changes

@JungMinu JungMinu JungMinu approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http2 Issues or PRs related to the http2 subsystem. needs-ci PRs 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