Fix simplify reply sent monitoring

[sergejostir]

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

After another review, I noticed a problem with my previous PR #3072.

I figured out that when http2 is used and the request method is "HEAD", response.finished will be always true (my understanding after reading https://nodejs.org/api/http2.html#http2_response_finished was that it behaves the same way as response.writableEnded, since that is suggested as a replacement). This is problematic with Node.js versions prior to 12.9.0 where response.writableEnded does not exist.

#2233 was made to cope with this problem, but it relied on response.writable, which by my testing will stay true even after the response has ended, so we cannot use it as an indicator.

Therefore, I added additional check, so now both response.finished as well as response.headersSent has to be true in order for reply to be considered sent. This change only applies for Node.js versions prior to 12.9.0 because newer versions will be using response.writableEnded which works correctly.

I also removed additional checks from preValidationCallback and preHandlerCallback that I missed, which were introduced by #2233. They are not needed any more with the new way that the reply.sent works.

PS: The test/http2/head.test.js was supposed to fail before on node10, but I only noticed that in my local environment today. Therefore I didn't include any new tests, since the part that this PR targets is already covered by an existing test.

I'm sorry for any inconvenience caused.

[jsumners]

Why weren't any tests failing in CI before this change?

[Eomm]

PS: The test/http2/head.test.js was supposed to fail before on node10, but I only noticed that in my local environment today. Therefore I didn't include any new tests, since the part that this PR targets is already covered by an existing test.

There is this PR that is failing #2862 on that test file. I was trying it locally but it passes so I was really confused about that tho

I don't know why my pipeline was failing and the other one no 😞

[mcollina]

lgtm

[mcollina]

@Eomm I don't understand, do you approve this change?

[sergejostir]

Why weren't any tests failing in CI before this change?

They were, but due to unrelated reason https://github.com/fastify/fastify/actions/runs/846451395. CI should be manually re-run I guess.

[Eomm]

@Eomm I don't understand, do you approve this change?

No, I didn't get the chance to check this PR deeply, I was pointing out that this PR should fix the other one pipeline 🤞🏽

[jsumners]

Why weren't any tests failing in CI before this change?

They were, but due to unrelated reason fastify/fastify/actions/runs/846451395. CI should be manually re-run I guess.

GitHub Actions was having some issues at the time the initial PR was being considered. Unfortunately, we merged without waiting for those to be resolved and re-running the jobs to make sure everything was green. I just re-ran them, and sure enough, the HTTP2 tests fail -- https://github.com/fastify/fastify/pull/3072/checks?check_run_id=2600626970#step:6:9795

[jsumners]

Looks good to me.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.