| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/167/pipeline |
Sorry, something went wrong.
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
There was a problem hiding this comment.
Code SGTM but I couldn't reproduce on my Mac so I can't confirm the fix.
Sorry, something went wrong.
|
Thanks for the reviews – I’m labelling this as in progress to make sure we don’t land it until somebody has had time to look into why the test is flaky/how to fix it … We probably need to just attach a no-op error handler somewhere, but I couldn’t reproduce the issue on Linux |
Sorry, something went wrong.
|
@addaleax I think you're right that a no-op error handler on the client side should help with the flakiness. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Yes! Its work for me, fast reload not crush node |
Sorry, something went wrong.
|
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… |
Sorry, something went wrong.
|
Maybe ping @nodejs/http2 ? I guess alternatively I’ll ask for access to a build machine … |
Sorry, something went wrong.
|
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 error event about half the time, so this test fails the other half of the time. Not sure why. stream.on('error', common.mustCall((error) => {
const expected = 'ECONNRESET'
assert.strictEqual(error.errno, expected);
assert.strictEqual(error.code, expected);
})); |
Sorry, something went wrong.
|
New CI to see on which hosts exactly it’s failing (?): https://ci.nodejs.org/job/node-test-pull-request/15809/ |
Sorry, something went wrong.
|
The socket destroy is what causes the ECONNRESET. I don't really see a good way to get around it but I'll try to dig in today & tomorrow. (The test might need to be rewritten to work on macOS from what I can tell.) |
Sorry, something went wrong.
|
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. :/) |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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