Looking at the http1 semantic the current http2 compat implementation of finished seems rather complicated and fragile? Also it doesn't match the semantics of http1.

I have one http2 compat test failing which I don't quite undertand the purpose of. Am I missing something critical here?

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

Refs: #24743

NOTE TO SELF: review cb behaviour when ending and destroyed. Is ´onStreamCloseResponse` required?

The interesting part here is that in http1 finished doesn't seem to care whether the socket is closed or not, it only cares about whether end has been called or not. While in http2-compat it doesn't really care that much about end, instead if mostly cares about whether the socket is closed or not.

I think this discrepancy is also missing a test somewhere? @mcollina

EDIT: See, fab97ee

The interesting part here is that in http1 finished doesn't seem to care whether the socket is closed or not, it only cares about whether end has been called or not. While in http2-compat it doesn't really care that much about end, instead if mostly cares about whether the socket is closed or not.

I think this discrepancy is also missing a test somewhere? @mcollina

I think this is just a difference that we need to fix.

I'm actually a bit unsure what happens if you call end on a closed connection in http1?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually a bit unsure what happens if you call end on a closed connection in http1?

if the stream is finished, nothing happens on multiple end() in HTTP1

If you are still unsure, have you trying preparing an example?

@mcollina

if the stream is finished, nothing happens on multiple end() in HTTP1

This is if you have called end() previously. But what if you call end for the first time after the socket has aborted.

Can you create an example to check this?

Sorry @mcollina, I just spotted the notification on this. I'll queue this one up and take a look hopefully later today.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apapirovski care to expand on that?

The failing test looks wrong for me:

const server = createServer(mustCall((request, response) => {
    strictEqual(response.finished, true);
    response.writeHead(HTTP_STATUS_OK, { foo: 'bar' });
    response.end('data', mustCall());
  }));

It test that respone.finished is true before end() is called which is not how http/1 behaves.

review cb behaviour when ending and destroyed. Is ´onStreamCloseResponse` required?

Yup, it is. We need to trigger those events in the HEAD case, as well as cleanup the proxy socket and other junk since otherwise they're mutually referencing each other and could cause memory leaks.

Would this be a bug fix or semver-major?

@Trott: CI failed due to merge conflict. Please try again.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nodejs/build I seem to be having problems with node-test-binary-arm-12+ in multiple PR's. Even those that just touch docs? #31805

Please do not use the "Resume build" when the PR changes. The "Resume build" only starts jobs that failed, so jobs that passed before will remain green, even if they would fail with the new changes. The "Resume build" feature is a source of problems, but it's the only way we have to reduce the impact of infra failures.

Please use "Rebuild" instead when there are any changes in the PR.

Both e6a54b6 and db24c25 were the head of #31805 at some point. Only the node-test-binary-arm-12+ job was started in https://ci.nodejs.org/job/node-test-pull-request/29176/. The other two were re-used from a previous PR because they passed, so the compile job never ran for the latest head.