-
Notifications
You must be signed in to change notification settings - Fork 30k
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
node_http2.cc fatal error #33156
Comments
I've actually been investigating this one and I think I'm close to a fix on it. |
I got this in node 14.12, Centos 7
|
We used node 12.4 for so long because of #33875, Now we upgraded to 14.12 and after two days, this exception has happened more than 4 times and caused so many problems, for example all pending DB updates gone. I wonder why an error in one of the http2 streams should kill the whole process and can't even be catched in uncaughtException. @jasnell Anyway as it seems there is no interest in resolving this issue, I just want to know if it's ok to replace this assertion with a normal if check in the source code so node continues to work? |
@jasnell As that was April not sure if you're still working on this. If not, would you mind sharing your conclusions/analysis and/or your half-finished fix attempt, so others could try to progress this if you no longer have the time? Many thanks 🙂 |
Seeing this too but only on Node 12:
|
Fix and test here: https://github.com/nodejs/node/compare/v12.19.1...davedoesdev:issue-33156-http2-close-while-writing?expand=1 I can't seem to PR against 12.19.1 It's fixed on Node 15 due to (However, making that change doesn't make Node 12 pass, doesn't seem quite to follow the same path/timings) |
Wouldn't harm to make the equivalent fix on Node 15 and then backport. There might be a case where it triggers but I couldn't get a test that did so. |
@Trott Thanks for merging the fix to master. Do I need to do anything to get the fix to Node 12 merged? |
Unfortunately, 83166fb doesn't currently cherry-pick cleanly over to the v12.x-staging branch. Unless something else lands first to make it cherry-pick cleanly there, it will need a backport PR. If you don't mind doing the work to put that together, the instructions are at https://github.com/nodejs/node/blob/1ed72f67f5ea82b36b8589e447619e98c004fa12/doc/guides/backporting-to-release-lines.md. |
Thanks, I'll look at doing it soon. |
Fixes: nodejs#33156 PR-URL: nodejs#36241 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
What steps will reproduce the bug?
I use simple http2 server for my site. It worked fine for about 1 week and today I got fatal exeption. Seems problem in Http2Session module.
How often does it reproduce? Is there a required condition?
What is the expected behavior?
What do you see instead?
Additional information
The text was updated successfully, but these errors were encountered: