-
Notifications
You must be signed in to change notification settings - Fork 75
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 early event loop exit on nodejs when H2 sessions are in the verify state #861
Conversation
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.
LGTM.
Can you update the PR title description with more details? They are basically our changelog, and we should take the extra couple of minutes it takes to make it really easy to see what's changed from release notes, with a link to a PR providing details.
For example, this PR title:
Fix early event loop exit on Node.js when H2 sessions are in the
verify
state
... would be more likely to ring a bell for a user who saw an unexpected early exit in their client app.
@@ -2,3 +2,4 @@ | |||
/*.json | |||
/dist/*/**/*.spec.js | |||
/dist/*/**/*.spec.d.ts | |||
/dist/*/testdata/ |
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.
🙏
it("verify should keep the process alive", async function () { | ||
const worker = new Worker( |
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.
Great idea to use a worker instead of a child process. I don't see an alternative to test the actual outcome, and I think it's worth it.
ping
Fix early event loop exit on nodejs when H2 sessions are in the verify state. We
unref
the connections to avoid idle connections stopping the process from exiting. This results in a case where process exits when verification is required (last pinged time elapsed ping interval). We alwaysref
before a manualping
andunref
if it succeeds with empty streams.Used a worker to test the behavior. Not super happy about it but seemed better than a child process.
Fixes #860.