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>

Author: mcollina

doc/api/http2.md

@@ -2906,9 +2906,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
@@ -3127,9 +3128,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
@@ -3307,9 +3309,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;
@@ -1561,7 +1569,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

@@ -500,9 +500,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,69 @@
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const Countdown = require('../common/countdown');
+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(() => {
+    let destroyed = false;
+    const countdown = new Countdown(totalRequests, common.mustCall(() => {
+      assert(rejected > 0);
+      assert(accepted < totalRequests);
+      server.close();
+    }));
+
+    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++;
+          if (!destroyed) {
+            destroyed = true;
+            client.destroy();
+          }
+        }
+        countdown.dec();
+      });
+      req.end();
+    }
+  }));
+}));