Skip to content
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

[v16.x backport] src: let http2 streams end after session close #45660

Closed
wants to merge 1 commit into from
Closed

[v16.x backport] src: let http2 streams end after session close #45660

wants to merge 1 commit into from

Conversation

adamrdavid
Copy link

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. v16.x labels Nov 28, 2022
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 28, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 28, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Nov 29, 2022
Open
16 tasks
@santigimeno @adamrdavid
src: let http2 streams end after session close
8bb8590 
After the stream has been marked as closed by the nghttp2 stack, there
might be still pending data to be sent: trailing headers is an example
of this. In that case, avoid reentering the nghttp2 stack synchronously
to allow the data to be written before actually closing the stream.

Fixes: nodejs#42713
PR-URL: nodejs#45153
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
(cherry picked from commit 71bdecb)
@adamrdavid adamrdavid marked this pull request as ready for review November 29, 2022 15:19
This was referenced Nov 30, 2022
Open
Open
Open
Open
Open
Open
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 7, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 7, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Dec 8, 2022
Open
8 tasks
richardlau pushed a commit that referenced this pull request Dec 8, 2022
@santigimeno @richardlau
src: let http2 streams end after session close
953072d 
After the stream has been marked as closed by the nghttp2 stack, there
might be still pending data to be sent: trailing headers is an example
of this. In that case, avoid reentering the nghttp2 stack synchronously
to allow the data to be written before actually closing the stream.

Fixes: #42713
PR-URL: #45153
Backport-PR-URL: #45660
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@richardlau
Copy link
Member

Landed in 953072d.

@richardlau richardlau closed this Dec 8, 2022
@adamrdavid adamrdavid deleted the backport-45153 branch December 8, 2022 16:07
@adamrdavid
Copy link
Author

Thank you for landing @richardlau, does that mean this will release with 2022-12-13 noted in nodejs/Release#658 ?

@richardlau
Copy link
Member

@adamrdavid That's the plan, yes.

guangwong pushed a commit to noslate-project/node that referenced this pull request Jan 3, 2023
@santigimeno @guangwong
src: let http2 streams end after session close
bae75ef 
After the stream has been marked as closed by the nghttp2 stack, there
might be still pending data to be sent: trailing headers is an example
of this. In that case, avoid reentering the nghttp2 stack synchronously
to allow the data to be written before actually closing the stream.

Fixes: nodejs/node#42713
PR-URL: nodejs/node#45153
Backport-PR-URL: nodejs/node#45660
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Jan 3, 2023
@santigimeno @guangwong
src: let http2 streams end after session close
07bb1d8 
After the stream has been marked as closed by the nghttp2 stack, there
might be still pending data to be sent: trailing headers is an example
of this. In that case, avoid reentering the nghttp2 stack synchronously
to allow the data to be written before actually closing the stream.

Fixes: nodejs/node#42713
PR-URL: nodejs/node#45153
Backport-PR-URL: nodejs/node#45660
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. 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.
5 participants
@adamrdavid @nodejs-github-bot @richardlau @RafaelGSS @santigimeno