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.

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

http2: close stream and session on frameError #42147

Conversation

Copy link
Member

@RafaelGSS RafaelGSS commented Feb 28, 2022

Address: #35133.

From the Http2Stream#frameError event documentation:

The 'frameError' event is emitted when an error occurs while attempting to send a frame. When invoked, the handler function will receive an integer argument identifying the frame type, and an integer argument identifying the error code. The Http2Stream instance will be destroyed immediately after the 'frameError' event is emitted.

The master implementation wasn't closing the stream/session when a frameError occurs. Also, it was calling the frameError event with an nghttp2 internal error code.

This PR translates the internal error code according to RFC 7540 and closes the connection when a frameError occurs.

@nodejs-github-bot
Copy link

@nodejs-github-bot nodejs-github-bot commented Feb 28, 2022

Review requested:

@nodejs-github-bot nodejs-github-bot added c++ http2 needs-ci labels Feb 28, 2022
@RafaelGSS RafaelGSS force-pushed the fix/close-stream-after-frame-error branch from 3ff2d7b to 3f3dde8 Compare Feb 28, 2022
@RafaelGSS RafaelGSS changed the title fix: close stream and session on frameError http2: close stream and session on frameError Feb 28, 2022
@Trott
Copy link

@Trott Trott commented Feb 28, 2022

Copy link
Member

@mcollina mcollina left a comment

lgtm

@nodejs-github-bot
Copy link

@nodejs-github-bot nodejs-github-bot commented Feb 28, 2022

@nodejs-github-bot
Copy link

@nodejs-github-bot nodejs-github-bot commented Feb 28, 2022

@mcollina mcollina added the commit-queue label Mar 2, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue label Mar 2, 2022
@nodejs-github-bot nodejs-github-bot merged commit db6490c into nodejs:master Mar 2, 2022
51 checks passed
@nodejs-github-bot
Copy link

@nodejs-github-bot nodejs-github-bot commented Mar 2, 2022

Landed in db6490c

sxa pushed a commit to sxa/node that referenced this issue Mar 7, 2022
PR-URL: nodejs#42147
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@sxa sxa mentioned this pull request Mar 8, 2022
danielleadams pushed a commit to danielleadams/node that referenced this issue Apr 21, 2022
PR-URL: nodejs#42147
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit that referenced this issue Apr 24, 2022
PR-URL: #42147
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit that referenced this issue Apr 24, 2022
PR-URL: #42147
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit that referenced this issue Apr 24, 2022
PR-URL: #42147
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
xtx1130 pushed a commit to xtx1130/node that referenced this issue Apr 25, 2022
PR-URL: nodejs#42147
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ http2 needs-ci
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants