http2: fix issues with aborted respondWithFile()s

[addaleax]

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), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[nodejs-github-bot]

@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

[addaleax]

Full CI: https://ci.nodejs.org/job/node-test-pull-request/15649/

[addaleax]

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?

[apapirovski]

Code SGTM but I couldn't reproduce on my Mac so I can't confirm the fix.

[DaAitch]

@addaleax cool, could not reproduce crashes.

[ryzokuken]

@addaleax I'll check if the test I wrote still crashes (highly unlikely), but code-wise LGTM.

[addaleax]

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

[jasnell]

@addaleax I think you're right that a no-op error handler on the client side should help with the flakiness.

[apapirovski]

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.

[ariran5]

Yes! Its work for me, fast reload not crush node

[addaleax]

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…

[addaleax]

Maybe ping @nodejs/http2 ?

I guess alternatively I’ll ask for access to a build machine …

[sebdeckers]

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);
  }));

[addaleax]

New CI to see on which hosts exactly it’s failing (?): https://ci.nodejs.org/job/node-test-pull-request/15809/

[apapirovski]

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.)

[addaleax]

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: https://ci.nodejs.org/job/node-test-pull-request/15888/

[addaleax]

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.

[mcollina]

LGTM

[addaleax]

Landed in d94950e