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

Running benchmark/http/headers.js fails with HPE_HEADER_OVERFLOW #31022

Closed
trivikr opened this issue Dec 18, 2019 · 5 comments
Closed

Running benchmark/http/headers.js fails with HPE_HEADER_OVERFLOW #31022

trivikr opened this issue Dec 18, 2019 · 5 comments
Labels
benchmark Issues and PRs related to the benchmark subsystem. http Issues or PRs related to the http subsystem.

Comments

@trivikr
Copy link
Member

trivikr commented Dec 18, 2019

  • Version: master, v12.14.0, v10.18.0
  • Platform: Ubuntu
  • Subsystem: http

Describe the bug
Running benchmark/http/headers.js fails with HPE_HEADER_OVERFLOW for:

  • master
  • v12.14.0
  • v10.18.0

To Reproduce
Follow instructions in running benchmarks to run benchmark/http/headers.js

Example:

$ node -v
v12.14.0

$ node benchmark/http/headers.js 
http/headers.js len=1 n=10 benchmarker="test-double-http": 15,309
http/headers.js len=100 n=10 benchmarker="test-double-http": 1,974
events.js:187
      throw er; // Unhandled 'error' event
      ^

Error: Parse Error: Header overflow
    at Socket.socketOnData (_http_client.js:456:22)
    at Socket.emit (events.js:210:5)
    at addChunk (_stream_readable.js:309:12)
    at readableAddChunk (_stream_readable.js:290:11)
    at Socket.Readable.push (_stream_readable.js:224:10)
    at TCP.onStreamRead (internal/stream_base_commons.js:182:23)
Emitted 'error' event on ClientRequest instance at:
    at Socket.socketOnData (_http_client.js:463:9)
    at Socket.emit (events.js:210:5)
    [... lines matching original stack trace ...]
    at TCP.onStreamRead (internal/stream_base_commons.js:182:23) {
  bytesParsed: 9458,
  code: 'HPE_HEADER_OVERFLOW',
  reason: 'Header overflow',
  rawPacket: <Buffer 48 54 54 50 2f 31 2e 31 20 32 30 30 20 4f 4b 0d 0a 43 6f 6e 6e 65 63 74 69 6f 6e 3a 20 6b 65 65 70 2d 61 6c 69 76 65 0d 0a 54 72 61 6e 73 66 65 72 2d ... 21838 more bytes>
}
Error: test-double-http failed with 1.
    at ChildProcess.<anonymous> (/home/trivikr/workspace/node/benchmark/_http-benchmarkers.js:229:16)
    at Object.onceWrapper (events.js:300:26)
    at ChildProcess.emit (events.js:210:5)
    at maybeClose (internal/child_process.js:1021:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:283:5)

$ nvm use 10
Now using node v10.18.0 (npm v6.13.4)

$ node benchmark/http/headers.js 
http/headers.js len=1 n=10 benchmarker="test-double-http": 1,658
http/headers.js len=100 n=10 benchmarker="test-double-http": 1,678
events.js:174
      throw er; // Unhandled 'error' event
      ^

Error: Parse Error
    at Socket.socketOnData (_http_client.js:442:20)
    at Socket.emit (events.js:198:13)
    at addChunk (_stream_readable.js:287:12)
    at readableAddChunk (_stream_readable.js:268:11)
    at Socket.Readable.push (_stream_readable.js:223:10)
    at TCP.onStreamRead (internal/stream_base_commons.js:94:17)
Emitted 'error' event at:
    at Socket.socketOnData (_http_client.js:448:9)
    at Socket.emit (events.js:198:13)
    [... lines matching original stack trace ...]
    at TCP.onStreamRead (internal/stream_base_commons.js:94:17)
Error: test-double-http failed with 1.
    at ChildProcess.child.once (/home/trivikr/workspace/node/benchmark/_http-benchmarkers.js:229:16)
    at Object.onceWrapper (events.js:286:20)
    at ChildProcess.emit (events.js:198:13)
    at maybeClose (internal/child_process.js:982:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:259:5)

Expected behavior
The benchmark/http/headers.js runs without any error

Additional context
I came across this issue as part of running benchmarks while moving to for...of loop in the initial commits of #30958

@trivikr
Copy link
Member Author

trivikr commented Dec 18, 2019

The benchmark succeeds if I install autocannon

$ node -v
v12.14.0

$ npm install -g autocannon
/home/trivikr/.nvm/versions/node/v12.14.0/bin/autocannon -> /home/trivikr/.nvm/versions/node/v12.14.0/lib/node_modules/autocannon/autocannon.js
+ [email protected]
added 41 packages from 70 contributors in 1.456s

$ node benchmark/http/headers.js 
http/headers.js len=1 n=10 benchmarker="autocannon": 42,550.4
http/headers.js len=100 n=10 benchmarker="autocannon": 16,156
http/headers.js len=1 n=1000 benchmarker="autocannon": 1,933.4
http/headers.js len=100 n=1000 benchmarker="autocannon": 2,021.8

Should we improve the error code if wrk or autocannon is not installed? https://github.com/nodejs/node/blob/651c43082698ba3bfa4f2417944719f771c09f04/benchmark/writing-and-running-benchmarks.md#http-benchmark-requirements

@Trott
Copy link
Member

Trott commented Dec 18, 2019

The benchmark succeeds if I install autocannon

autocannon or wrk will generally take precedence over test-double-http in these benchmarks.

@targos
Copy link
Member

targos commented Dec 26, 2020

What should be done here?

@targos targos added benchmark Issues and PRs related to the benchmark subsystem. http Issues or PRs related to the http subsystem. labels Dec 26, 2020
@Trott
Copy link
Member

Trott commented Jan 5, 2021

More than two years ago, for security/reliability purposes, the default maximum header size was reduce from 80Kb to 8192 bytes. We can either reduce the size of the benchmark so it fits inside the default or else pass the --max-http-header-size flag to the child process. I think reducing the size of the benchmark probably makes sense. It seems like headers about 8192 bytes are an edge case with some valid uses but not something we necessarily want to be optimizing for.

@Trott
Copy link
Member

Trott commented Jan 5, 2021

Proposed fix in #36794

@Trott Trott closed this as completed in 1e35d58 Jan 7, 2021
danielleadams pushed a commit that referenced this issue Jan 12, 2021
The http/headers.js benchmark fails if wrk or autocannon are not
installed. This is because the benchmark exceeds the default permissible
length for HTTP headers. Reduce the largest benchmarks to fit within the
8192 default as that is what we should be optimizing for.

Fixes: #31022

PR-URL: #36794
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
targos pushed a commit that referenced this issue May 1, 2021
The http/headers.js benchmark fails if wrk or autocannon are not
installed. This is because the benchmark exceeds the default permissible
length for HTTP headers. Reduce the largest benchmarks to fit within the
8192 default as that is what we should be optimizing for.

Fixes: #31022

PR-URL: #36794
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
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. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants