-
Notifications
You must be signed in to change notification settings - Fork 18
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
base: master
Are you sure you want to change the base?
Conversation
207e275
to
51eeaf5
Compare
@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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
No worries, I'll create one |
Thank you! Not familiar with the ava testing framework. |
Closes #89