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

Handle GOAWAY Frame #89

Open
mkaufmaner opened this issue Oct 21, 2022 · 1 comment · May be fixed by #90
Open

Handle GOAWAY Frame #89

mkaufmaner opened this issue Oct 21, 2022 · 1 comment · May be fixed by #90

Comments

@mkaufmaner
Copy link

Currently the http2-wrapper agent instantiates a new ClientHttp2Session with http2.connect in the Agent class. After this session is created a listener is attached for the error event here and the close event here.

However, there is no listener for the goaway event. Since there is no handler to remove sessions which they will continue to be used by subsequent requests. This will then lead to the New streams cannot be created after receiving a GOAWAY error being thrown because the session that received the GOAWAY has not been not removed from the map of available sessions.

See https://github.com/nodejs/node/blob/v16.x/lib/internal/http2/core.js#L1166

// Receiving a GOAWAY frame will cause the Http2Session to first emit a 'goaway'
// event notifying the user that a shutdown is in progress. If the goaway
// error code equals 0 (NGHTTP2_NO_ERROR), session.close() will be called,
// causing the Http2Session to send its own GOAWAY frame and switch itself
// into a graceful closing state. In this state, new inbound or outbound
// Http2Streams will be rejected. Existing *pending* streams (those created
// but without an assigned stream ID or handle) will be destroyed with a
// cancel error. Existing open streams will be permitted to complete on their
// own. Once all existing streams close, session.destroy() will be called
// automatically.

Related to sindresorhus/got#2156

@gal-monday
Copy link

Any ETA when it's going to be merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants