feat: http version check by climba03003 · Pull Request #3912 · fastify/fastify

climba03003 · 2022-05-19T08:35:03Z

Fixes #3893

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

github-actions · 2022-05-19T08:38:10Z

Node: 14
PR: [1] 1488k requests in 30.06s, 280 MB read
MAIN: [1] 1478k requests in 30.06s, 278 MB read

Node: 16
PR: [1] 1595k requests in 30.06s, 300 MB read
MAIN: [1] 1605k requests in 30.07s, 302 MB read

Node: 18
PR: [1] 1723k requests in 30.04s, 324 MB read
MAIN: [1] 1785k requests in 30.04s, 335 MB read

climba03003 · 2022-05-19T08:49:14Z

I think the impact is minimal.

Pull Request

$ npx concurrently -k -s first "node ./examples/benchmark/simple.js" "npx autocannon -c 100 -d 30 -p 10 localhost:3000/"
[1] Running 30s test @ http://localhost:3000/
[1] 100 connections with 10 pipelining factor
[1]
[1] 
[1] ┌─────────┬───────┬───────┬───────┬───────┬──────────┬─────────┬────────┐
[1] │ Stat    │ 2.5%  │ 50%   │ 97.5% │ 99%   │ Avg      │ Stdev   │ Max    │
[1] ├─────────┼───────┼───────┼───────┼───────┼──────────┼─────────┼────────┤
[1] │ Latency │ 20 ms │ 23 ms │ 35 ms │ 46 ms │ 24.43 ms │ 4.86 ms │ 138 ms │
[1] └─────────┴───────┴───────┴───────┴───────┴──────────┴─────────┴────────┘
[1] ┌───────────┬─────────┬─────────┬────────┬─────────┬──────────┬─────────┬─────────┐
[1] │ Stat      │ 1%      │ 2.5%    │ 50%    │ 97.5%   │ Avg      │ Stdev   │ Min     │
[1] ├───────────┼─────────┼─────────┼────────┼─────────┼──────────┼─────────┼─────────┤
[1] │ Req/Sec   │ 32095   │ 32095   │ 40415  │ 41791   │ 40134.67 │ 1724.15 │ 32093   │
[1] ├───────────┼─────────┼─────────┼────────┼─────────┼──────────┼─────────┼─────────┤
[1] │ Bytes/Sec │ 6.04 MB │ 6.04 MB │ 7.6 MB │ 7.86 MB │ 7.55 MB  │ 323 kB  │ 6.03 MB │
[1] └───────────┴─────────┴─────────┴────────┴─────────┴──────────┴─────────┴─────────┘
[1]
[1] Req/Bytes counts sampled once per second.
[1] # of samples: 30
[1]
[1] 1205k requests in 30.05s, 226 MB read
[1] npx autocannon -c 100 -d 30 -p 10 localhost:3000/ exited with code 0
--> Sending SIGTERM to other processes..
[0] node ./examples/benchmark/simple.js exited with code SIGTERM
Done in 33.95s.

Main Branch

$ npx concurrently -k -s first "node ./examples/benchmark/simple.js" "npx autocannon -c 100 -d 30 -p 10 localhost:3000/"
[1] Running 30s test @ http://localhost:3000/
[1] 100 connections with 10 pipelining factor
[1]
[1] 
[1] ┌─────────┬───────┬───────┬───────┬───────┬──────────┬─────────┬────────┐    
[1] │ Stat    │ 2.5%  │ 50%   │ 97.5% │ 99%   │ Avg      │ Stdev   │ Max    │    
[1] ├─────────┼───────┼───────┼───────┼───────┼──────────┼─────────┼────────┤    
[1] │ Latency │ 20 ms │ 23 ms │ 37 ms │ 46 ms │ 24.18 ms │ 5.28 ms │ 146 ms │    
[1] └─────────┴───────┴───────┴───────┴───────┴──────────┴─────────┴────────┘    
[1] ┌───────────┬────────┬────────┬────────┬───────┬─────────┬─────────┬────────┐
[1] │ Stat      │ 1%     │ 2.5%   │ 50%    │ 97.5% │ Avg     │ Stdev   │ Min    │
[1] ├───────────┼────────┼────────┼────────┼───────┼─────────┼─────────┼────────┤
[1] │ Req/Sec   │ 31407  │ 31407  │ 40991  │ 42527 │ 40535.2 │ 1913.72 │ 31395  │
[1] ├───────────┼────────┼────────┼────────┼───────┼─────────┼─────────┼────────┤
[1] │ Bytes/Sec │ 5.9 MB │ 5.9 MB │ 7.7 MB │ 8 MB  │ 7.62 MB │ 360 kB  │ 5.9 MB │
[1] └───────────┴────────┴────────┴────────┴───────┴─────────┴─────────┴────────┘
[1]
[1] Req/Bytes counts sampled once per second.
[1] # of samples: 30
[1]
[1] 1217k requests in 30.05s, 229 MB read
[1] npx autocannon -c 100 -d 30 -p 10 localhost:3000/ exited with code 0
--> Sending SIGTERM to other processes..
[0] node ./examples/benchmark/simple.js exited with code SIGTERM
Done in 33.96s.

Fdawgs · 2022-05-19T08:56:14Z

Would nodejs/node#43115, as mentioned in the other PR, not tackle this?

climba03003 · 2022-05-19T08:58:32Z

Would nodejs/node#43115, as mentioned in the other PR, not tackle this?

It can handle it, but the that issue seems no activity.
At least, I don't think it will land in a near future.

If the impact is small enough (fall between the variation), I think we can land it first.
Then, remove it after the core update.

jsumners · 2022-05-19T14:20:59Z

What issue does this solve?

climba03003 · 2022-05-19T14:22:11Z

What issue does this solve?

#3893

RafaelGSS

LGTM.

mcollina

lgtm

jsumners · 2022-05-20T13:49:01Z

+  return function validateHTTPVersion (httpVersion) {
+    // `bypass` skip the check when custom server factory provided
+    // `httpVersion in obj` check for the valid http version we should support
+    return bypass || httpVersion in obj


What would the difference be in using an actual Map and then a .has check?

Suggested change

return bypass || httpVersion in obj

return bypass || obj.has(httpVersion)

~~Map is definitely slower in this case.~~
I also tested Set, it is about the same as Array.includes.

I re-run the benchmark. Map actually about the same as in operator.

using Map.has x 1,295,809,605 ops/sec ±0.09% (98 runs sampled) using Map.has (first item) x 1,293,557,094 ops/sec ±0.12% (96 runs sampled) using Set.has x 145,554,007 ops/sec ±0.65% (91 runs sampled) using Set.has (first item) x 147,771,297 ops/sec ±0.31% (96 runs sampled) using in obj x 1,294,797,802 ops/sec ±0.19% (94 runs sampled) using in obj (first item) x 1,295,422,306 ops/sec ±0.05% (100 runs sampled) using Array.includes x 127,497,843 ops/sec ±0.51% (95 runs sampled) using Array.includes (first item) x 148,103,651 ops/sec ±0.26% (95 runs sampled) Using raw comparisson x 512,871,599 ops/sec ±35.76% (39 runs sampled) Using raw comparisson (first item) x 145,327,537 ops/sec ±4.66% (79 runs sampled)

Co-authored-by: James Sumners <[email protected]>

Co-authored-by: Frazer Smith <[email protected]>

jsumners

Looks good to me.

mcollina · 2022-05-21T14:45:40Z

Can you resolve the conflicts?

github-actions · 2023-05-22T00:22:41Z

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.

feat: http version check

79bb611

climba03003 added the benchmark Label to run benchmark against PR and main branch label May 19, 2022

github-actions Bot removed the benchmark Label to run benchmark against PR and main branch label May 19, 2022

refactor: extract function

3aa3f19

climba03003 mentioned this pull request May 19, 2022

Fix 3893: return 505 if the request http version is not supported #3909

Closed

4 tasks

test: update test

0a6c606

fix: wrong condition

167d747

p16 mentioned this pull request May 19, 2022

http2 request to a http1 fastify server hits "default handler for 404 did not catch this, this is likely a fastify bug" #3893

Closed

2 tasks

climba03003 marked this pull request as ready for review May 19, 2022 09:12

climba03003 commented May 19, 2022

View reviewed changes

Comment thread lib/server.js Outdated

jsumners reviewed May 19, 2022

View reviewed changes

Comment thread lib/route.js

chore: add todo note

1874ce5

RafaelGSS approved these changes May 19, 2022

View reviewed changes

Eomm approved these changes May 19, 2022

View reviewed changes

mcollina reviewed May 19, 2022

View reviewed changes

Comment thread lib/server.js

climba03003 added 2 commits May 20, 2022 10:37

chore: explain how compileValidateHTTPVersion works

dc1f334

docs: http version check

2eebe35

climba03003 requested a review from mcollina May 20, 2022 02:51

mcollina approved these changes May 20, 2022

View reviewed changes

jsumners reviewed May 20, 2022

View reviewed changes

Comment thread docs/Reference/Routes.md Outdated

jsumners requested changes May 20, 2022

View reviewed changes

climba03003 and others added 2 commits May 20, 2022 21:55

docs: keep consistence

ec0b938

chore: update comment

af4351e

Co-authored-by: James Sumners <[email protected]>

climba03003 requested a review from jsumners May 20, 2022 13:58

refactor: replace to map

7422432

jsumners reviewed May 20, 2022

View reviewed changes

Comment thread lib/server.js Outdated

climba03003 added 2 commits May 20, 2022 22:23

docs: lint wrap

f33e2be

chore: update comment

95195b2

climba03003 requested a review from Fdawgs May 20, 2022 14:35

Fdawgs requested changes May 20, 2022

View reviewed changes

Comment thread docs/Reference/Routes.md Outdated

Comment thread lib/server.js Outdated

chore: update comment

a344e30

Co-authored-by: Frazer Smith <[email protected]>

Fdawgs approved these changes May 20, 2022

View reviewed changes

jsumners approved these changes May 20, 2022

View reviewed changes

Merge branch 'main' into http-version-check

52848a8

mcollina merged commit ad0d31a into main May 21, 2022

mcollina deleted the http-version-check branch May 21, 2022 18:09

github-actions Bot locked as resolved and limited conversation to collaborators May 22, 2023

	return bypass \|\| httpVersion in obj
	return bypass \|\| obj.has(httpVersion)

Uh oh!

Conversation

climba03003 commented May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

github-actions Bot commented May 19, 2022

Uh oh!

climba03003 commented May 19, 2022

Uh oh!

Fdawgs commented May 19, 2022

Uh oh!

climba03003 commented May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jsumners commented May 19, 2022

Uh oh!

climba03003 commented May 19, 2022

Uh oh!

Uh oh!

RafaelGSS left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jsumners May 20, 2022

Choose a reason for hiding this comment

Uh oh!

climba03003 May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsumners left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented May 21, 2022

Uh oh!

github-actions Bot commented May 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

climba03003 commented May 19, 2022 •

edited

Loading

climba03003 commented May 19, 2022 •

edited

Loading

climba03003 May 20, 2022 •

edited

Loading