Skip to content

http2: refactor Http2Stream and inbound headers #16676

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

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 30 additions & 3 deletions doc/api/http2.md
Original file line number Diff line number Diff line change
Expand Up @@ -1470,11 +1470,18 @@ not be emitted.
### http2.createServer(options[, onRequestHandler])
<!-- YAML
added: v8.4.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/16676
description: Added the `maxHeaderListPairs` option with a default limit of
128 header pairs.
-->

* `options` {Object}
* `maxDeflateDynamicTableSize` {number} Sets the maximum dynamic table size
for deflating header fields. **Default:** `4Kib`
* `maxHeaderListPairs` {number} Sets the maximum number of header entries.
**Default:** `128`. The minimum value is `4`.
* `maxSendHeaderBlockLength` {number} Sets the maximum allowed size for a
serialized, compressed block of headers. Attempts to send headers that
exceed this limit will result in a `'frameError'` event being emitted
Expand Down Expand Up @@ -1525,6 +1532,11 @@ server.listen(80);
### http2.createSecureServer(options[, onRequestHandler])
<!-- YAML
added: v8.4.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/16676
description: Added the `maxHeaderListPairs` option with a default limit of
128 header pairs.
-->

* `options` {Object}
Expand All @@ -1533,6 +1545,8 @@ added: v8.4.0
`false`. See the [`'unknownProtocol'`][] event. See [ALPN negotiation][].
* `maxDeflateDynamicTableSize` {number} Sets the maximum dynamic table size
for deflating header fields. **Default:** `4Kib`
* `maxHeaderListPairs` {number} Sets the maximum number of header entries.
**Default:** `128`. The minimum value is `4`.
* `maxSendHeaderBlockLength` {number} Sets the maximum allowed size for a
serialized, compressed block of headers. Attempts to send headers that
exceed this limit will result in a `'frameError'` event being emitted
Expand Down Expand Up @@ -1590,12 +1604,19 @@ server.listen(80);
### http2.connect(authority[, options][, listener])
<!-- YAML
added: v8.4.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/16676
description: Added the `maxHeaderListPairs` option with a default limit of
128 header pairs.
-->

* `authority` {string|URL}
* `options` {Object}
* `maxDeflateDynamicTableSize` {number} Sets the maximum dynamic table size
for deflating header fields. **Default:** `4Kib`
* `maxHeaderListPairs` {number} Sets the maximum number of header entries.
**Default:** `128`. The minimum value is `1`.
* `maxReservedRemoteStreams` {number} Sets the maximum number of reserved push
streams the client will accept at any given time. Once the current number of
currently reserved push streams exceeds reaches this limit, new push streams
Expand Down Expand Up @@ -1747,7 +1768,13 @@ server.on('stream', (stream, headers) => {
```

### Settings Object

<!-- YAML
added: v8.4.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/16676
description: The `maxHeaderListSize` setting is now strictly enforced.
-->
The `http2.getDefaultSettings()`, `http2.getPackedSettings()`,
`http2.createServer()`, `http2.createSecureServer()`,
`http2session.settings()`, `http2session.localSettings`, and
Expand All @@ -1773,8 +1800,8 @@ properties.
concurrently at any given time in an `Http2Session`. The minimum value is
0. The maximum allowed value is 2<sup>31</sup>-1.
* `maxHeaderListSize` {number} Specifies the maximum size (uncompressed octets)
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point


All additional properties on the settings object are ignored.

Expand Down
8 changes: 7 additions & 1 deletion lib/internal/http2/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,8 @@ const IDX_OPTIONS_MAX_RESERVED_REMOTE_STREAMS = 1;
const IDX_OPTIONS_MAX_SEND_HEADER_BLOCK_LENGTH = 2;
const IDX_OPTIONS_PEER_MAX_CONCURRENT_STREAMS = 3;
const IDX_OPTIONS_PADDING_STRATEGY = 4;
const IDX_OPTIONS_FLAGS = 5;
const IDX_OPTIONS_MAX_HEADER_LIST_PAIRS = 5;
const IDX_OPTIONS_FLAGS = 6;

function updateOptionsBuffer(options) {
var flags = 0;
Expand Down Expand Up @@ -201,6 +202,11 @@ function updateOptionsBuffer(options) {
optionsBuffer[IDX_OPTIONS_PADDING_STRATEGY] =
options.paddingStrategy;
}
if (typeof options.maxHeaderListPairs === 'number') {
flags |= (1 << IDX_OPTIONS_MAX_HEADER_LIST_PAIRS);
optionsBuffer[IDX_OPTIONS_MAX_HEADER_LIST_PAIRS] =
options.maxHeaderListPairs;
}
optionsBuffer[IDX_OPTIONS_FLAGS] = flags;
}

Expand Down
31 changes: 21 additions & 10 deletions src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "node_http2_state.h"

#include <queue>
#include <algorithm>

namespace node {

Expand All @@ -20,8 +21,6 @@ using v8::Undefined;

namespace http2 {

Freelist<Nghttp2Stream, FREELIST_MAX> stream_free_list;

Nghttp2Session::Callbacks Nghttp2Session::callback_struct_saved[2] = {
Callbacks(false),
Callbacks(true)};
Expand Down Expand Up @@ -67,6 +66,10 @@ Http2Options::Http2Options(Environment* env) {
buffer.GetValue(IDX_OPTIONS_PADDING_STRATEGY));
SetPaddingStrategy(strategy);
}

if (flags & (1 << IDX_OPTIONS_MAX_HEADER_LIST_PAIRS)) {
SetMaxHeaderPairs(buffer[IDX_OPTIONS_MAX_HEADER_LIST_PAIRS]);
}
}

Http2Settings::Http2Settings(Environment* env) : env_(env) {
Expand Down Expand Up @@ -173,11 +176,14 @@ inline void Http2Settings::RefreshDefaults(Environment* env) {
DEFAULT_SETTINGS_INITIAL_WINDOW_SIZE;
buffer[IDX_SETTINGS_MAX_FRAME_SIZE] =
DEFAULT_SETTINGS_MAX_FRAME_SIZE;
buffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE] =
DEFAULT_SETTINGS_MAX_HEADER_LIST_SIZE;
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, this is … clever 😄

}


Expand All @@ -192,7 +198,10 @@ Http2Session::Http2Session(Environment* env,

padding_strategy_ = opts.GetPaddingStrategy();

Init(type, *opts);
int32_t maxHeaderPairs = opts.GetMaxHeaderPairs();
maxHeaderPairs = type == NGHTTP2_SESSION_SERVER ?
std::max(maxHeaderPairs, 4) : std::max(maxHeaderPairs, 1);
Init(type, *opts, nullptr, maxHeaderPairs);

// For every node::Http2Session instance, there is a uv_prepare_t handle
// whose callback is triggered on every tick of the event loop. When
Expand Down Expand Up @@ -911,7 +920,8 @@ void Http2Session::OnTrailers(Nghttp2Stream* stream,

void Http2Session::OnHeaders(
Nghttp2Stream* stream,
std::queue<nghttp2_header>* headers,
nghttp2_header* headers,
size_t count,
nghttp2_headers_category cat,
uint8_t flags) {
Local<Context> context = env()->context();
Expand All @@ -936,18 +946,19 @@ void Http2Session::OnHeaders(
// like {name1: value1, name2: value2, name3: [value3, value4]}. We do it
// this way for performance reasons (it's faster to generate and pass an
// array than it is to generate and pass the object).
do {
size_t n = 0;
while (count > 0) {
size_t j = 0;
while (!headers->empty() && j < arraysize(argv) / 2) {
nghttp2_header item = headers->front();
while (count > 0 && j < arraysize(argv) / 2) {
nghttp2_header item = headers[n++];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// The header name and value are passed as external one-byte strings
name_str =
ExternalHeader::New<true>(env(), item.name).ToLocalChecked();
value_str =
ExternalHeader::New<false>(env(), item.value).ToLocalChecked();
argv[j * 2] = name_str;
argv[j * 2 + 1] = value_str;
headers->pop();
count--;
j++;
}
// For performance, we pass name and value pairs to array.protototype.push
Expand All @@ -956,7 +967,7 @@ void Http2Session::OnHeaders(
if (j > 0) {
fn->Call(env()->context(), holder, j * 2, argv).ToLocalChecked();
}
} while (!headers->empty());
}

Local<Value> args[4] = {
Integer::New(isolate, stream->id()),
Expand Down
25 changes: 12 additions & 13 deletions src/node_http2.h
Original file line number Diff line number Diff line change
Expand Up @@ -291,14 +291,6 @@ const char* nghttp2_errname(int rv) {
}
}

#define DEFAULT_SETTINGS_HEADER_TABLE_SIZE 4096
#define DEFAULT_SETTINGS_ENABLE_PUSH 1
#define DEFAULT_SETTINGS_INITIAL_WINDOW_SIZE 65535
#define DEFAULT_SETTINGS_MAX_FRAME_SIZE 16384
#define MAX_MAX_FRAME_SIZE 16777215
#define MIN_MAX_FRAME_SIZE DEFAULT_SETTINGS_MAX_FRAME_SIZE
#define MAX_INITIAL_WINDOW_SIZE 2147483647

// This allows for 4 default-sized frames with their frame headers
static const size_t kAllocBufferSize = 4 * (16384 + 9);

Expand All @@ -323,19 +315,25 @@ class Http2Options {
return options_;
}

void SetMaxHeaderPairs(uint32_t max) {
max_header_pairs_ = max;
}

uint32_t GetMaxHeaderPairs() const {
return max_header_pairs_;
}

void SetPaddingStrategy(padding_strategy_type val) {
#if DEBUG
CHECK_LE(val, PADDING_STRATEGY_CALLBACK);
#endif
padding_strategy_ = static_cast<padding_strategy_type>(val);
}

padding_strategy_type GetPaddingStrategy() {
padding_strategy_type GetPaddingStrategy() const {
return padding_strategy_;
}

private:
nghttp2_option* options_;
uint32_t max_header_pairs_ = DEFAULT_MAX_HEADER_LIST_PAIRS;
padding_strategy_type padding_strategy_ = PADDING_STRATEGY_NONE;
};

Expand Down Expand Up @@ -412,7 +410,8 @@ class Http2Session : public AsyncWrap,

void OnHeaders(
Nghttp2Stream* stream,
std::queue<nghttp2_header>* headers,
nghttp2_header* headers,
size_t count,
nghttp2_headers_category cat,
uint8_t flags) override;
void OnStreamClose(int32_t id, uint32_t code) override;
Expand Down
Loading