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

This PR simplifies "reply.sent" monitoring; instead of manually setting the kReplySent property all over the package (in more than 10 different places!), it relies on node's response.writableEnded (since fastify supports node versions prior to 12.9.0 which added "writableEnded", reponse.finished is used as a fallback).
When the user sets "reply.sent = true" or uses reply.hijack(), the kReplySentOverwritten property is now used.

You can see that all existing tests (except for one which I had to negligibly modify because it didn't use the proper object) are passing, therefore I believe no changes for the end users were introduced.

[jsumners]

Tests fail due to decreased coverage -- https://github.com/fastify/fastify/pull/3072/checks?check_run_id=2585550350#step:6:11141

[sergejostir]

Tests fail due to decreased coverage -- https://github.com/fastify/fastify/pull/3072/checks?check_run_id=2585550350#step:6:11141

Do you have any suggestion for solving this? I believe it's because of

https://github.com/sergiz/fastify/blob/8e61a472e6d091fab328e425d23ecbb663ed6b2e/lib/reply.js#L85

[RafaelGSS]

Can you do a benchmark compare?

[jsumners]

Tests fail due to decreased coverage -- #3072 (checks)

Do you have any suggestion for solving this? I believe it's because of

sergiz/fastify@8e61a47/lib/reply.js#L85

Have you run the HTML coverage report to see what is not being covered?

[sergejostir]

Have you run the HTML coverage report to see what is not being covered?

I installed the node v10 and the console already showed me that that line was problematic. I included two more tests now which cover both possibilities.

Can you do a benchmark compare?

How should I do that? I ran npm run benchmark a few times and the results are in the same range which makes sense since the only change that this PR does is to read from a different property.

[RafaelGSS]

How should I do that? I ran npm run benchmark a few times and the results are in the same range which makes sense since the only change that this PR does is to read from a different property.

Yes, npm run bench and compare main with your branch. Can you share the results?

[sergejostir]

Yes, npm run bench and compare main with your branch. Can you share the results?

main

[1] Running 5s test @ http://localhost:3000/
[1] 100 connections with 10 pipelining factor
[1] 
[1] ┌─────────┬───────┬───────┬───────┬───────┬──────────┬──────────┬────────┐
[1] │ Stat    │ 2.5%  │ 50%   │ 97.5% │ 99%   │ Avg      │ Stdev    │ Max    │
[1] ├─────────┼───────┼───────┼───────┼───────┼──────────┼──────────┼────────┤
[1] │ Latency │ 11 ms │ 24 ms │ 42 ms │ 73 ms │ 22.12 ms │ 11.36 ms │ 195 ms │
[1] └─────────┴───────┴───────┴───────┴───────┴──────────┴──────────┴────────┘
[1] ┌───────────┬─────────┬─────────┬─────────┬─────────┬─────────┬─────────┬─────────┐
[1] │ Stat      │ 1%      │ 2.5%    │ 50%     │ 97.5%   │ Avg     │ Stdev   │ Min     │
[1] ├───────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
[1] │ Req/Sec   │ 31631   │ 31631   │ 47295   │ 49151   │ 44257.6 │ 6511.52 │ 31617   │
[1] ├───────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
[1] │ Bytes/Sec │ 5.91 MB │ 5.91 MB │ 8.85 MB │ 9.19 MB │ 8.28 MB │ 1.22 MB │ 5.91 MB │
[1] └───────────┴─────────┴─────────┴─────────┴─────────┴─────────┴─────────┴─────────┘
[1] 
[1] Req/Bytes counts sampled once per second.
[1] 
[1] 221k requests in 5.04s, 41.4 MB read
[1] npx autocannon -c 100 -d 5 -p 10 localhost:3000/ exited with code 0

[1] Running 5s test @ http://localhost:3000/
[1] 100 connections with 10 pipelining factor
[1] 
[1] ┌─────────┬──────┬───────┬───────┬───────┬──────────┬──────────┬────────┐
[1] │ Stat    │ 2.5% │ 50%   │ 97.5% │ 99%   │ Avg      │ Stdev    │ Max    │
[1] ├─────────┼──────┼───────┼───────┼───────┼──────────┼──────────┼────────┤
[1] │ Latency │ 9 ms │ 21 ms │ 41 ms │ 66 ms │ 20.64 ms │ 10.53 ms │ 123 ms │
[1] └─────────┴──────┴───────┴───────┴───────┴──────────┴──────────┴────────┘
[1] ┌───────────┬─────────┬─────────┬────────┬─────────┬─────────┬─────────┬────────┐
[1] │ Stat      │ 1%      │ 2.5%    │ 50%    │ 97.5%   │ Avg     │ Stdev   │ Min    │
[1] ├───────────┼─────────┼─────────┼────────┼─────────┼─────────┼─────────┼────────┤
[1] │ Req/Sec   │ 35839   │ 35839   │ 48671  │ 54815   │ 47344   │ 6295.15 │ 35838  │
[1] ├───────────┼─────────┼─────────┼────────┼─────────┼─────────┼─────────┼────────┤
[1] │ Bytes/Sec │ 6.71 MB │ 6.71 MB │ 9.1 MB │ 10.3 MB │ 8.85 MB │ 1.18 MB │ 6.7 MB │
[1] └───────────┴─────────┴─────────┴────────┴─────────┴─────────┴─────────┴────────┘
[1] 
[1] Req/Bytes counts sampled once per second.
[1] 
[1] 237k requests in 5.04s, 44.3 MB read
[1] npx autocannon -c 100 -d 5 -p 10 localhost:3000/ exited with code 0

pr/simplify-reply-sent-monitoring

[1] Running 5s test @ http://localhost:3000/
[1] 100 connections with 10 pipelining factor
[1] 
[1] ┌─────────┬───────┬───────┬───────┬───────┬──────────┬──────────┬────────┐
[1] │ Stat    │ 2.5%  │ 50%   │ 97.5% │ 99%   │ Avg      │ Stdev    │ Max    │
[1] ├─────────┼───────┼───────┼───────┼───────┼──────────┼──────────┼────────┤
[1] │ Latency │ 10 ms │ 21 ms │ 45 ms │ 67 ms │ 21.65 ms │ 11.53 ms │ 188 ms │
[1] └─────────┴───────┴───────┴───────┴───────┴──────────┴──────────┴────────┘
[1] ┌───────────┬─────────┬─────────┬─────────┬─────────┬─────────┬─────────┬─────────┐
[1] │ Stat      │ 1%      │ 2.5%    │ 50%     │ 97.5%   │ Avg     │ Stdev   │ Min     │
[1] ├───────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
[1] │ Req/Sec   │ 33855   │ 33855   │ 47455   │ 52127   │ 45206.4 │ 6339.41 │ 33843   │
[1] ├───────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
[1] │ Bytes/Sec │ 6.33 MB │ 6.33 MB │ 8.88 MB │ 9.75 MB │ 8.45 MB │ 1.19 MB │ 6.33 MB │
[1] └───────────┴─────────┴─────────┴─────────┴─────────┴─────────┴─────────┴─────────┘
[1] 
[1] Req/Bytes counts sampled once per second.
[1] 
[1] 226k requests in 5.04s, 42.3 MB read
[1] npx autocannon -c 100 -d 5 -p 10 localhost:3000/ exited with code 0

[1] Running 5s test @ http://localhost:3000/
[1] 100 connections with 10 pipelining factor
[1] 
[1] ┌─────────┬──────┬───────┬───────┬───────┬──────────┬──────────┬────────┐
[1] │ Stat    │ 2.5% │ 50%   │ 97.5% │ 99%   │ Avg      │ Stdev    │ Max    │
[1] ├─────────┼──────┼───────┼───────┼───────┼──────────┼──────────┼────────┤
[1] │ Latency │ 9 ms │ 20 ms │ 39 ms │ 63 ms │ 20.41 ms │ 10.77 ms │ 150 ms │
[1] └─────────┴──────┴───────┴───────┴───────┴──────────┴──────────┴────────┘
[1] ┌───────────┬─────────┬─────────┬─────────┬─────────┬─────────┬─────────┬─────────┐
[1] │ Stat      │ 1%      │ 2.5%    │ 50%     │ 97.5%   │ Avg     │ Stdev   │ Min     │
[1] ├───────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
[1] │ Req/Sec   │ 35359   │ 35359   │ 50687   │ 54655   │ 47862.4 │ 6787.31 │ 35351   │
[1] ├───────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
[1] │ Bytes/Sec │ 6.61 MB │ 6.61 MB │ 9.48 MB │ 10.2 MB │ 8.95 MB │ 1.27 MB │ 6.61 MB │
[1] └───────────┴─────────┴─────────┴─────────┴─────────┴─────────┴─────────┴─────────┘
[1] 
[1] Req/Bytes counts sampled once per second.
[1] 
[1] 239k requests in 5.04s, 44.7 MB read
[1] npx autocannon -c 100 -d 5 -p 10 localhost:3000/ exited with code 0

[mcollina]

lgtm

I'm a bit nervous about this change as it might have some unplanned behavior change and break people code. I guess we can revert in that case.

[RafaelGSS]

LGTM.

[RafaelGSS]

Can you rebase to main to run the CI again?

[sergejostir]

Can you rebase to main to run the CI again?

Done.

[zekth]

Lgtm

[mcollina]

Unfortunately this is a breaking change, I'll revert on main and reapply it on next.

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