Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http2 benchmark seems broken #36746

Closed
aduh95 opened this issue Jan 2, 2021 · 4 comments · Fixed by #36871
Closed

http2 benchmark seems broken #36746

aduh95 opened this issue Jan 2, 2021 · 4 comments · Fixed by #36871
Labels
benchmark Issues and PRs related to the benchmark subsystem. http2 Issues or PRs related to the http2 subsystem.

Comments

@aduh95
Copy link
Contributor

aduh95 commented Jan 2, 2021

  • Test: http2/compat.js http2/simple.js
  • Platform: benchmark-node-micro-benchmarks
  • Console Output:
"old", "http2/compat.js", "duration=5 benchmarker='test-double-http2' clients=2 streams=100 requests=1000", 2024, 5.239167185
/w/bnch-comp/node/benchmark/_test-double-benchmarker.js:49
    client.on('error', (e) => { throw e; });
                                ^

Error: connect EADDRNOTAVAIL 127.0.0.1:12346 - Local (127.0.0.1:0)
    at internalConnect (node:net:911:16)
    at defaultTriggerAsyncIdScope (node:internal/async_hooks:430:12)
    at node:net:1002:9
    at processTicksAndRejections (node:internal/process/task_queues:76:11) {
  errno: -99,
  code: 'EADDRNOTAVAIL',
  syscall: 'connect',
  address: '127.0.0.1',
  port: 12346
}
Error: test-double-http2 failed with 1.
    at ChildProcess.<anonymous> (/w/bnch-comp/node/benchmark/_http-benchmarkers.js:242:16)
    at Object.onceWrapper (node:events:486:26)
    at ChildProcess.emit (node:events:379:20)
    at maybeClose (node:internal/child_process:1063:16)
    at Socket.<anonymous> (node:internal/child_process:449:11)
    at Socket.emit (node:events:379:20)
    at Pipe.<anonymous> (node:net:667:12)
@targos targos added benchmark Issues and PRs related to the benchmark subsystem. http2 Issues or PRs related to the http2 subsystem. labels Jan 3, 2021
@Trott
Copy link
Member

Trott commented Jan 10, 2021

Not having this problem locally. I wonder if what's going on is that port 12346 is used by the test suite and the server is running tests at the same time as the http2 benchmark. This would seem to be borne out by the fact that it fails with the issue at different times in the test suite. Another possibility is that the benchmark isn't waiting for the previous server to fully close before opening the next one.

@Trott
Copy link
Member

Trott commented Jan 10, 2021

I was able to replicate this locally. It just took much much longer. And I wasn't doing anything else on the computer, so it's something within the benchmark itself apparently and not an interaction with a test suite. Maybe _http-benchmarkers.js closes the server somewhere but doesn't wait for a callback.

@Trott
Copy link
Member

Trott commented Jan 10, 2021

I think I see the bug in the benchmarks themselves. They call close() but don't wait for the callback, so the next benchmark can start too soon. I think the problem took longer to happen locally for me because it is probably more likely to happen on a fast machine than a slow one and the CI machine is probably very fast.

I'll open a PR to fix this by using port 0 for the benchmarks so that we fix this issue without shutdown time becoming part of the benchmark measurement.

@Trott
Copy link
Member

Trott commented Jan 10, 2021

Proposed fix is in #36871

@Trott Trott closed this as completed in 1c4fa9a Jan 11, 2021
danielleadams pushed a commit that referenced this issue Jan 12, 2021
Fixes: #36746

PR-URL: #36871
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
targos pushed a commit that referenced this issue May 1, 2021
Fixes: #36746

PR-URL: #36871
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants