http2: [compat api] allow 'aborted' event on Http2ServerResponse instances and trigger those on session.socket's 'sessionError'

[rexagod]

EDIT: Removed Fixes: #28267 in accordance to #32937 (review)

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[mcollina]

cc @nodejs/tsc

[mcollina]

cc @nodejs/tsc

[mcollina]

LGTM

[mcollina]

@nodejs/http2 @nodejs/tsc PTAL

[mcollina]

cc @jasnell

[ronag]

I think the idea of #28267 was that 'aborted' should be emitted on the http2 compat response object, not the server.

[rexagod]

@ronag @mcollina I've reverted old changes and made some new ones. Please review.

[ronag]

Can you add a test that checks that if you get a sessionError with ECONNRESET that also means you get an aborted on the outgoing object?

[rexagod]

@ronag How did you invoke the socketOnError method? I'm unable to reproduce an ECONNRESET error. I tried to explicitly emit error events on ClientHttp2Session.sockets but since sockets aren't allowed to be manipulated, I get an ERR_HTTP2_NO_SOCKET_MANIPULATION error, not the ECONNRESET one. 😕

[BridgeAR]

@ronag PTAL

[ronag]

@ronag How did you invoke the socketOnError method?

I don't remember. Was a long time ago. I probably got it in production somewhere.

[ronag]

I'm not sure this is how HTTP/1 behaves. Please see comment and confirm. If that's not how HTTP/1 behaves it might make sense to make it so as part of a semver-major.

Also, this doesn't actually solve #28267 so we should probably remove the reference.

[ronag]

Can you confirm that the corresponding code for HTTP1 would actually emit 'aborted' here?

[rexagod]

@ronag ServerResponse did not emit 'aborted' with the following code.

// differences from the HTTP/2 corresponding code
// IncomingMessage: did not emit 'error', emitted only 'aborted' event
// ServerResponse: did not emit neither 'error' nor 'aborted' events
const server = http.createServer(common.mustCall((req, res) => {
  // req.on('error', common.mustCall()) // fails
  // res.on('error', common.mustCall()) // fails
  req.on('aborted', common.mustCall(() => {
    assert.ok(req.aborted); // passes
  }));
  // res.on('aborted', common.mustCall()) // fails
  res.write('xyz');
  server.close();
})).listen(0, common.mustCall(function () {
  const request = http.get({
    path: '/',
    port: server.address().port
  }, function(res) {
    res.on('data', common.mustCall((d) => {
      request.destroy();
    }));
  });
}));

[rexagod]

Ping @ronag

[ronag]

Given #32937 (comment). What is this fixing? The way I see it this actually diverges from the http/1 behavior?

[rexagod]

@ronag Feel free to close this if you think that's the best option here.

However, at this point, I'm interested in the best approach we can take to fix #28267 given its irreproducibility?

[rexagod]

Nonetheless, I've run into ECONNRESET a few times, just not sure if this was emitted from the same method we're talking about. I'll take another go at this in a separate PR soon.