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

fix: set session as gracefully closing on goaway frame #90

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mkaufmaner
Copy link

Closes #89

@mkaufmaner mkaufmaner reopened this Oct 27, 2022
@mkaufmaner mkaufmaner marked this pull request as ready for review October 27, 2022 20:30
@mkaufmaner
Copy link
Author

@szmarczak I am not really sure how to create a test for this specific use case, but I will continue to try.

source/agent.js Outdated
session.once('goaway', () => {
// Prevent session from being used for new requests.
// The session will eventually emit either an 'error' or 'close' event.
session[kGracefullyClosing] = true;
Copy link
Owner

Choose a reason for hiding this comment

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

Please use gracefullyClose(session) instead

Copy link
Owner

Choose a reason for hiding this comment

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

Can you add a comment to the spec as well please?

Copy link
Author

Choose a reason for hiding this comment

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

@szmarczak My only concern with using gracefullyClose(session) is that session.close() may be fired multiple times but this condition appears to be handled here so I will update it to use gracefullyClose.

As for the spec, do you mean the comment in the source code?

Copy link
Owner

Choose a reason for hiding this comment

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

No, I mean the HTTP/2 RFC

Copy link
Author

Choose a reason for hiding this comment

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

Does this work?

// See https://datatracker.ietf.org/doc/html/rfc7540#section-6.8 

// There is an inherent race condition between an endpoint starting new streams and the remote sending a GOAWAY frame.
// Receivers of a GOAWAY frame MUST NOT open additional streams on the connection, although a new connection can be established for new streams.

Copy link
Author

Choose a reason for hiding this comment

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

Change made to reference RFC

Copy link
Author

Choose a reason for hiding this comment

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

@szmarczak Do you need me to do anything else for this?

@szmarczak
Copy link
Owner

No worries, I'll create one

@mkaufmaner
Copy link
Author

No worries, I'll create one

Thank you! Not familiar with the ava testing framework.

dav1d8 added a commit to dav1d8/node-http-proxy that referenced this pull request Mar 3, 2023
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 this pull request may close these issues.

Handle GOAWAY Frame
2 participants