http2: retain header memory in session accounting

Author: mcollina

doc/api/http2.md

@@ -2874,9 +2874,10 @@ changes:
     This is a credit based limit, existing `Http2Stream`s may cause this
     limit to be exceeded, but new `Http2Stream` instances will be rejected
     while this limit is exceeded. The current number of `Http2Stream` sessions,
-    the current memory use of the header compression tables, current data
-    queued to be sent, and unacknowledged `PING` and `SETTINGS` frames are all
-    counted towards the current limit. **Default:** `10`.
+    the current memory use of the header compression tables, header blocks
+    retained by open streams, current data queued to be sent, and
+    unacknowledged `PING` and `SETTINGS` frames are all counted towards the
+    current limit. **Default:** `10`.
   * `maxHeaderListPairs` {number} Sets the maximum number of header entries.
     This is similar to [`server.maxHeadersCount`][] or
     [`request.maxHeadersCount`][] in the `node:http` module. The minimum value
@@ -3095,9 +3096,10 @@ changes:
     credit based limit, existing `Http2Stream`s may cause this
     limit to be exceeded, but new `Http2Stream` instances will be rejected
     while this limit is exceeded. The current number of `Http2Stream` sessions,
-    the current memory use of the header compression tables, current data
-    queued to be sent, and unacknowledged `PING` and `SETTINGS` frames are all
-    counted towards the current limit. **Default:** `10`.
+    the current memory use of the header compression tables, header blocks
+    retained by open streams, current data queued to be sent, and
+    unacknowledged `PING` and `SETTINGS` frames are all counted towards the
+    current limit. **Default:** `10`.
   * `maxHeaderListPairs` {number} Sets the maximum number of header entries.
     This is similar to [`server.maxHeadersCount`][] or
     [`request.maxHeadersCount`][] in the `node:http` module. The minimum value
@@ -3275,9 +3277,10 @@ changes:
     This is a credit based limit, existing `Http2Stream`s may cause this
     limit to be exceeded, but new `Http2Stream` instances will be rejected
     while this limit is exceeded. The current number of `Http2Stream` sessions,
-    the current memory use of the header compression tables, current data
-    queued to be sent, and unacknowledged `PING` and `SETTINGS` frames are all
-    counted towards the current limit. **Default:** `10`.
+    the current memory use of the header compression tables, header blocks
+    retained by open streams, current data queued to be sent, and
+    unacknowledged `PING` and `SETTINGS` frames are all counted towards the
+    current limit. **Default:** `10`.
   * `maxHeaderListPairs` {number} Sets the maximum number of header entries.
     This is similar to [`server.maxHeadersCount`][] or
     [`request.maxHeadersCount`][] in the `node:http` module. The minimum value

src/node_http2.cc

@@ -902,6 +902,14 @@ BaseObjectPtr<Http2Stream> Http2Session::RemoveStream(int32_t id) {
   stream = FindStream(id);
   if (stream) {
     streams_.erase(id);
+    if (stream->current_headers_length_ > 0) {
+      DecrementCurrentSessionMemory(stream->current_headers_length_);
+      stream->current_headers_length_ = 0;
+    }
+    if (stream->retained_headers_length_ > 0) {
+      DecrementCurrentSessionMemory(stream->retained_headers_length_);
+      stream->retained_headers_length_ = 0;
+    }
     DecrementCurrentSessionMemory(sizeof(*stream));
   }
   return stream;
@@ -1548,7 +1556,10 @@ void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) {
   });
   CHECK_EQ(stream->headers_count(), 0);
 
-  DecrementCurrentSessionMemory(stream->current_headers_length_);
+  // Keep the header block charged against maxSessionMemory while the
+  // corresponding JS objects can still keep it alive for the lifetime of
+  // the stream.
+  stream->retained_headers_length_ += stream->current_headers_length_;
   stream->current_headers_length_ = 0;
 
   Local<Value> args[] = {

src/node_http2.h

@@ -490,9 +490,11 @@ class Http2Stream : public AsyncWrap,
 
   // The Current Headers block... As headers are received for this stream,
   // they are temporarily stored here until the OnFrameReceived is called
-  // signalling the end of the HEADERS frame
+  // signalling the end of the HEADERS frame.
   nghttp2_headers_category current_headers_category_ = NGHTTP2_HCAT_HEADERS;
   uint32_t current_headers_length_ = 0;  // total number of octets
+  // Charged against maxSessionMemory while headers stay alive in JS.
+  uint64_t retained_headers_length_ = 0;
   std::vector<Http2Header> current_headers_;
 
   // This keeps track of the amount of data read from the socket while the

test/parallel/test-http2-max-session-memory-stalled-headers.js

@@ -0,0 +1,62 @@
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const assert = require('assert');
+const http2 = require('http2');
+
+const {
+  NGHTTP2_ENHANCE_YOUR_CALM,
+} = http2.constants;
+
+// Regression test: header blocks retained by stalled streams should continue
+// to count against maxSessionMemory after they have been handed to JS.
+const maxSessionMemory = 1;
+const totalRequests = 400;
+const cookieCrumbs = 120;
+
+let accepted = 0;
+let rejected = 0;
+
+const server = http2.createServer({ maxSessionMemory });
+server.on('stream', (stream) => {
+  accepted++;
+  stream.on('error', () => {});
+  stream.respond();
+  stream.write('x');
+});
+
+server.listen(0, common.mustCall(() => {
+  const client = http2.connect(`http://localhost:${server.address().port}`, {
+    settings: {
+      initialWindowSize: 0,
+    },
+  });
+  client.on('error', () => {});
+
+  client.on('remoteSettings', common.mustCall(() => {
+    for (let i = 0; i < totalRequests; i++) {
+      const headers = [':path', '/'];
+      for (let j = 0; j < cookieCrumbs; j++) {
+        headers.push('cookie', 'a=1');
+      }
+
+      const req = client.request(headers);
+      req.on('error', () => {});
+      req.on('close', () => {
+        if (req.rstCode === NGHTTP2_ENHANCE_YOUR_CALM)
+          rejected++;
+      });
+      req.end();
+    }
+
+    setTimeout(common.mustCall(() => {
+      assert(rejected > 0);
+      assert(accepted < totalRequests);
+      client.destroy();
+      server.close();
+    }), common.platformTimeout(3000));
+  }));
+}));