Skip to content

Commit b7fd13a

Browse files
mcollinaaduh95
authored andcommitted
http2: retain header memory in session accounting
Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: #63752 Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
1 parent 34433a4 commit b7fd13a

4 files changed

Lines changed: 96 additions & 11 deletions

File tree

doc/api/http2.md

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2870,9 +2870,10 @@ changes:
28702870
This is a credit based limit, existing `Http2Stream`s may cause this
28712871
limit to be exceeded, but new `Http2Stream` instances will be rejected
28722872
while this limit is exceeded. The current number of `Http2Stream` sessions,
2873-
the current memory use of the header compression tables, current data
2874-
queued to be sent, and unacknowledged `PING` and `SETTINGS` frames are all
2875-
counted towards the current limit. **Default:** `10`.
2873+
the current memory use of the header compression tables, header blocks
2874+
retained by open streams, current data queued to be sent, and
2875+
unacknowledged `PING` and `SETTINGS` frames are all counted towards the
2876+
current limit. **Default:** `10`.
28762877
* `maxHeaderListPairs` {number} Sets the maximum number of header entries.
28772878
This is similar to [`server.maxHeadersCount`][] or
28782879
[`request.maxHeadersCount`][] in the `node:http` module. The minimum value
@@ -3087,9 +3088,10 @@ changes:
30873088
credit based limit, existing `Http2Stream`s may cause this
30883089
limit to be exceeded, but new `Http2Stream` instances will be rejected
30893090
while this limit is exceeded. The current number of `Http2Stream` sessions,
3090-
the current memory use of the header compression tables, current data
3091-
queued to be sent, and unacknowledged `PING` and `SETTINGS` frames are all
3092-
counted towards the current limit. **Default:** `10`.
3091+
the current memory use of the header compression tables, header blocks
3092+
retained by open streams, current data queued to be sent, and
3093+
unacknowledged `PING` and `SETTINGS` frames are all counted towards the
3094+
current limit. **Default:** `10`.
30933095
* `maxHeaderListPairs` {number} Sets the maximum number of header entries.
30943096
This is similar to [`server.maxHeadersCount`][] or
30953097
[`request.maxHeadersCount`][] in the `node:http` module. The minimum value
@@ -3267,9 +3269,10 @@ changes:
32673269
This is a credit based limit, existing `Http2Stream`s may cause this
32683270
limit to be exceeded, but new `Http2Stream` instances will be rejected
32693271
while this limit is exceeded. The current number of `Http2Stream` sessions,
3270-
the current memory use of the header compression tables, current data
3271-
queued to be sent, and unacknowledged `PING` and `SETTINGS` frames are all
3272-
counted towards the current limit. **Default:** `10`.
3272+
the current memory use of the header compression tables, header blocks
3273+
retained by open streams, current data queued to be sent, and
3274+
unacknowledged `PING` and `SETTINGS` frames are all counted towards the
3275+
current limit. **Default:** `10`.
32733276
* `maxHeaderListPairs` {number} Sets the maximum number of header entries.
32743277
This is similar to [`server.maxHeadersCount`][] or
32753278
[`request.maxHeadersCount`][] in the `node:http` module. The minimum value

src/node_http2.cc

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -902,6 +902,14 @@ BaseObjectPtr<Http2Stream> Http2Session::RemoveStream(int32_t id) {
902902
stream = FindStream(id);
903903
if (stream) {
904904
streams_.erase(id);
905+
if (stream->current_headers_length_ > 0) {
906+
DecrementCurrentSessionMemory(stream->current_headers_length_);
907+
stream->current_headers_length_ = 0;
908+
}
909+
if (stream->retained_headers_length_ > 0) {
910+
DecrementCurrentSessionMemory(stream->retained_headers_length_);
911+
stream->retained_headers_length_ = 0;
912+
}
905913
DecrementCurrentSessionMemory(sizeof(*stream));
906914
}
907915
return stream;
@@ -1548,7 +1556,10 @@ void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) {
15481556
});
15491557
CHECK_EQ(stream->headers_count(), 0);
15501558

1551-
DecrementCurrentSessionMemory(stream->current_headers_length_);
1559+
// Keep the header block charged against maxSessionMemory while the
1560+
// corresponding JS objects can still keep it alive for the lifetime of
1561+
// the stream.
1562+
stream->retained_headers_length_ += stream->current_headers_length_;
15521563
stream->current_headers_length_ = 0;
15531564

15541565
Local<Value> args[] = {

src/node_http2.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -490,9 +490,11 @@ class Http2Stream : public AsyncWrap,
490490

491491
// The Current Headers block... As headers are received for this stream,
492492
// they are temporarily stored here until the OnFrameReceived is called
493-
// signalling the end of the HEADERS frame
493+
// signalling the end of the HEADERS frame.
494494
nghttp2_headers_category current_headers_category_ = NGHTTP2_HCAT_HEADERS;
495495
uint32_t current_headers_length_ = 0; // total number of octets
496+
// Charged against maxSessionMemory while headers stay alive in JS.
497+
uint64_t retained_headers_length_ = 0;
496498
std::vector<Http2Header> current_headers_;
497499

498500
// This keeps track of the amount of data read from the socket while the
Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
if (!common.hasCrypto)
5+
common.skip('missing crypto');
6+
7+
const Countdown = require('../common/countdown');
8+
const assert = require('assert');
9+
const http2 = require('http2');
10+
11+
const {
12+
NGHTTP2_ENHANCE_YOUR_CALM,
13+
} = http2.constants;
14+
15+
// Regression test: header blocks retained by stalled streams should continue
16+
// to count against maxSessionMemory after they have been handed to JS.
17+
const maxSessionMemory = 1;
18+
const totalRequests = 400;
19+
const cookieCrumbs = 120;
20+
21+
let accepted = 0;
22+
let rejected = 0;
23+
24+
const server = http2.createServer({ maxSessionMemory });
25+
server.on('stream', (stream) => {
26+
accepted++;
27+
stream.on('error', () => {});
28+
stream.respond();
29+
stream.write('x');
30+
});
31+
32+
server.listen(0, common.mustCall(() => {
33+
const client = http2.connect(`http://localhost:${server.address().port}`, {
34+
settings: {
35+
initialWindowSize: 0,
36+
},
37+
});
38+
client.on('error', () => {});
39+
40+
client.on('remoteSettings', common.mustCall(() => {
41+
let destroyed = false;
42+
const countdown = new Countdown(totalRequests, common.mustCall(() => {
43+
assert(rejected > 0);
44+
assert(accepted < totalRequests);
45+
server.close();
46+
}));
47+
48+
for (let i = 0; i < totalRequests; i++) {
49+
const headers = [':path', '/'];
50+
for (let j = 0; j < cookieCrumbs; j++) {
51+
headers.push('cookie', 'a=1');
52+
}
53+
54+
const req = client.request(headers);
55+
req.on('error', () => {});
56+
req.on('close', () => {
57+
if (req.rstCode === NGHTTP2_ENHANCE_YOUR_CALM) {
58+
rejected++;
59+
if (!destroyed) {
60+
destroyed = true;
61+
client.destroy();
62+
}
63+
}
64+
countdown.dec();
65+
});
66+
req.end();
67+
}
68+
}));
69+
}));

0 commit comments

Comments
 (0)