http2: refactor Http2Stream and inbound headers #16676

jasnell · 2017-11-02T02:54:02Z

Eliminate pooling for Nghttp2Stream instances
Use a MaybeStackBuffer for inbound headers
Enforce a maximum number of headers per block
Enforce a maximum number of header octets per block using MAX_HEADERS_LIST setting

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

http2

jasnell · 2017-11-02T05:57:11Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11141/

mcollina · 2017-11-02T08:28:38Z

doc/api/http2.md

-  of header list that will be accepted. There is no default value. The minimum
-  allowed value is 0. The maximum allowed value is 2<sup>32</sup>-1.
+  of header list that will be accepted. The minimum allowed value is 0. The
+  maximum allowed value is 2<sup>32</sup>-1. **Default:** 65535.


we should probably update the changelog in the docs somehow for this.

mcollina · 2017-11-02T08:30:23Z

src/node_http2_core-inl.h

+  uint32_t maxHeaderListSize = session->GetMaxHeaderPairs();
+  if (maxHeaderListSize == 0)
+    maxHeaderListSize = DEFAULT_MAX_HEADER_LIST_PAIRS;
+  current_headers_.AllocateSufficientStorage(maxHeaderListSize);


does this mean we are preallocating an array of maxHeaderListSize? Do we need this to be 65535 as the default? IMHO we are wasting memory and time for each stream.

There are two limits: the max number of header pairs, and the max number of header octets. We are preallocating an array of nhgttp2_header structs. These consist of two pointers and an 8 bit flag. Current number is a generous 1024, which can likely be tuned much much lower. The 65535 is the total number of octets and is maintained as a size_t counter only. There's no preallocation for that.

We should be able to tune the number of header pairs down to something much much smaller than 1024.

I would go for 128

If you’re calling AllocateSufficientStorage(), you kind of shouldn’t be using a MaybeStackBuffer in the first place – all this will give you is unused memory inside the Nghttp2Stream instance.

I’d suggest just going with a std::vector here … you don’t need to care about pre-allocating memory in that case, plus you can still reserve memory ahead of time using e.g. vector.reserve(128).

yeah, I keep going back and forth on this, just playing with the various options to see which is going to provide the best performance. std::vector is likely the best approach based on everything I've seen.

mcollina · 2017-11-02T08:32:18Z

test/parallel/test-http2-too-many-headers.js

+// block is 1024, including the HTTP pseudo-header fields, by
+// setting one here, no request should be acccepted because it
+// does not allow for even the basic HTTP pseudo-headers
+const server = http2.createServer({ maxHeaderListPairs: 1 });


I think these types of settings should throw. What's the minimum?

We can set whatever minimum we want here. For the request to work, we need at least 4 to account for the required pseudoheaders.

I would say we validate it's greater than 4.

how about rather than report an error, we just automatically allow for the number of pseudo-headers? So if a user sets 0, the internal value is 4....

mcollina

LGTM with the default changed.

addaleax · 2017-11-02T21:31:56Z

src/node_http2.cc

  buffer[IDX_SETTINGS_COUNT] =
    (1 << IDX_SETTINGS_HEADER_TABLE_SIZE) |
    (1 << IDX_SETTINGS_ENABLE_PUSH) |
    (1 << IDX_SETTINGS_INITIAL_WINDOW_SIZE) |
-    (1 << IDX_SETTINGS_MAX_FRAME_SIZE);
+    (1 << IDX_SETTINGS_MAX_FRAME_SIZE) |
+    (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE);


btw, this is … clever 😄

addaleax · 2017-11-02T21:33:24Z

src/node_http2.cc

-    while (!headers->empty() && j < arraysize(argv) / 2) {
-      nghttp2_header item = headers->front();
+    while (count > 0 && j < arraysize(argv) / 2) {
+      nghttp2_header item = headers[n++];


Can’t this just be headers[--count]? I don’t think you really need the n if you didn’t need it before

addaleax · 2017-11-02T21:38:32Z

src/node_http2_core-inl.h

+  uint32_t maxHeaderListSize = session->GetMaxHeaderPairs();
+  if (maxHeaderListSize == 0)
+    maxHeaderListSize = DEFAULT_MAX_HEADER_LIST_PAIRS;
+  current_headers_.AllocateSufficientStorage(maxHeaderListSize);


If you’re calling AllocateSufficientStorage(), you kind of shouldn’t be using a MaybeStackBuffer in the first place – all this will give you is unused memory inside the Nghttp2Stream instance.

I’d suggest just going with a std::vector here … you don’t need to care about pre-allocating memory in that case, plus you can still reserve memory ahead of time using e.g. vector.reserve(128).

After testing, the pooling is not having any tangible benefit and makes things more complicated. Simplify. Simplify.

Enforce MAX_HEADERS_LIST setting and limit the number of header pairs accepted from the peer. Use the ENHANCE_YOUR_CALM error code when receiving either too many headers or too many octets. Use a MaybeStackBuffer to store the headers instead of a queue

jasnell · 2017-11-03T22:39:57Z

@mcollina @addaleax ... updated.

Set the default header pairs limit to 128
Updated so that on the server, 4 is the minimum. On the client, 1 is the minimum.
Updated to use a std::vector

mcollina

LGTM

jasnell · 2017-11-05T03:58:55Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11197/

* eliminate pooling of Nghttp2Stream instances. After testing, the pooling is not having any tangible benefit and makes things more complicated. Simplify. Simplify. * refactor inbound headers * Enforce MAX_HEADERS_LIST setting and limit the number of header pairs accepted from the peer. Use the ENHANCE_YOUR_CALM error code when receiving either too many headers or too many octets. Use a vector to store the headers instead of a queue PR-URL: #16676 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

jasnell · 2017-11-05T05:41:11Z

Landed in 9f3d59e

* eliminate pooling of Nghttp2Stream instances. After testing, the pooling is not having any tangible benefit and makes things more complicated. Simplify. Simplify. * refactor inbound headers * Enforce MAX_HEADERS_LIST setting and limit the number of header pairs accepted from the peer. Use the ENHANCE_YOUR_CALM error code when receiving either too many headers or too many octets. Use a vector to store the headers instead of a queue PR-URL: nodejs#16676 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

* eliminate pooling of Nghttp2Stream instances. After testing, the pooling is not having any tangible benefit and makes things more complicated. Simplify. Simplify. * refactor inbound headers * Enforce MAX_HEADERS_LIST setting and limit the number of header pairs accepted from the peer. Use the ENHANCE_YOUR_CALM error code when receiving either too many headers or too many octets. Use a vector to store the headers instead of a queue PR-URL: #16676 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 2, 2017

jasnell force-pushed the http2-refactor-stream branch from a26dbcf to fc76961 Compare November 2, 2017 02:58

mscdex added http2 Issues or PRs related to the http2 subsystem. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 2, 2017

jasnell requested review from addaleax, apapirovski, mcollina and sebdeckers November 2, 2017 05:56

mcollina reviewed Nov 2, 2017

View reviewed changes

mcollina approved these changes Nov 2, 2017

View reviewed changes

addaleax approved these changes Nov 2, 2017

View reviewed changes

jasnell force-pushed the http2-refactor-stream branch from 99ce733 to e067c58 Compare November 3, 2017 17:40

jasnell added 5 commits November 3, 2017 14:52

http2: eliminate pooling of Nghttp2Stream instances

8082939

After testing, the pooling is not having any tangible benefit and makes things more complicated. Simplify. Simplify.

http2: refactor inbound headers

54ce323

Enforce MAX_HEADERS_LIST setting and limit the number of header pairs accepted from the peer. Use the ENHANCE_YOUR_CALM error code when receiving either too many headers or too many octets. Use a MaybeStackBuffer to store the headers instead of a queue

[Squash] fix

2f78a9f

[Squash] Set header pairs limit to 128

957eff9

[Squash] Updates

59614d7

jasnell force-pushed the http2-refactor-stream branch from e067c58 to 59614d7 Compare November 3, 2017 22:37

doc: update docs with changes

c69e7b2

mcollina approved these changes Nov 4, 2017

View reviewed changes

jasnell closed this Nov 5, 2017

cjihrig mentioned this pull request Nov 6, 2017

v9.1.0 proposal #16851

Merged

MylesBorins added the dont-land-on-v6.x label Nov 14, 2017

MylesBorins mentioned this pull request Dec 7, 2017

v8.9.3 proposal #17532

Merged

himself65 mentioned this pull request Mar 24, 2020

Add maxHeadersCount support for http(s)2 server #32388

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2: refactor Http2Stream and inbound headers #16676

http2: refactor Http2Stream and inbound headers #16676

jasnell commented Nov 2, 2017

jasnell commented Nov 2, 2017

mcollina Nov 2, 2017

jasnell Nov 2, 2017

mcollina Nov 2, 2017

jasnell Nov 2, 2017

mcollina Nov 2, 2017

addaleax Nov 2, 2017

jasnell Nov 3, 2017

mcollina Nov 2, 2017

jasnell Nov 2, 2017

mcollina Nov 2, 2017

jasnell Nov 3, 2017

mcollina left a comment

addaleax Nov 2, 2017

addaleax Nov 2, 2017

addaleax Nov 2, 2017

jasnell commented Nov 3, 2017

mcollina left a comment

jasnell commented Nov 5, 2017

jasnell commented Nov 5, 2017

http2: refactor Http2Stream and inbound headers #16676

http2: refactor Http2Stream and inbound headers #16676

Conversation

jasnell commented Nov 2, 2017

Checklist

Affected core subsystem(s)

jasnell commented Nov 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Nov 3, 2017

mcollina left a comment

Choose a reason for hiding this comment

jasnell commented Nov 5, 2017

jasnell commented Nov 5, 2017