Skip to content

Conversation

@mkaufmaner
Copy link

Closes #89

@mkaufmaner mkaufmaner closed this Oct 27, 2022
@mkaufmaner mkaufmaner force-pushed the 89-handle-goaway-frame branch from 207e275 to 51eeaf5 Compare October 27, 2022 20:05
@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
@sindresorhus
Copy link

@szmarczak Bump :)

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

3 participants