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
http2: fix issues with aborted respondWithFile()s
#21561
Conversation
|
New CI: https://ci.nodejs.org/job/node-test-pull-request/15651/ @ariran5 Can you download the source for v10.5.0, apply https://patch-diff.githubusercontent.com/raw/nodejs/node/pull/21561.diff and build Node.js yourself? |
Code SGTM but I couldn't reproduce on my Mac so I can't confirm the fix.
|
Thanks for the reviews – I’m labelling this as |
|
@addaleax I think you're right that a no-op error handler on the client side should help with the flakiness. |
|
I can reproduce the failure reliably but don't know how to fix. The test as is doesn't seem to work on MacOS since the destroy causes the error. |
|
Yes! Its work for me, fast reload not crush node |
|
Could somebody from e.g. @nodejs/platform-macos help me figure out the test here? I don’t have easy access to any of the systems on which is is failing… |
|
Maybe ping @nodejs/http2 ? I guess alternatively I’ll ask for access to a build machine … |
|
FWIW I tried adding the following handler to the server in the test case. On macOS High Sierra (10.13.6) it fails to emit the stream.on('error', common.mustCall((error) => {
const expected = 'ECONNRESET'
assert.strictEqual(error.errno, expected);
assert.strictEqual(error.code, expected);
})); |
|
New CI to see on which hosts exactly it’s failing (?): https://ci.nodejs.org/job/node-test-pull-request/15809/ |
|
The socket destroy is what causes the (The test might need to be rewritten to work on macOS from what I can tell.) |
|
I’ve pushed a change that might help get this back on track, even if it’s just a workaround. If this is approved & lands with the hack in the test, I’ll open a new issue, since I think this is an API defect in the HTTP/2 API. (@apapirovski I still really appreciate you taking the time to do this – I’d assign that issue to you, if that’s okay, at least as long as I can’t really debug it myself. :/) |
|
CI is green. Can somebody take a look at the added changes (only 6a04dbe)? I think it’s okay to do this because it addresses the most important issue, which is that this should not hard-crash the process. |
|
Landed in d94950e |
Fixes: #20824
Fixes: #21560
@budarin @DaAitch @ariran5 Are you in a position to try this patch and see whether it fully resolves the issues you were experiencing?
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes