add 'maxHeaderSize' to 'http2'

[rexagod]

This is a feature request for adding maxHeaderSize in http2 as quoted below

---

Sounds good to me – maxHeaderSize could be added along with that. 👍

Originally posted by @addaleax in #32388 (comment)

[himself65]

-1 on this, as I said #32388 (comment)

[addaleax]

@himself65 I don’t see any issues with adding an alias for parity between HTTP/1 and HTTP/2. Having that will make things easier for users, and I think that’s worth the (very small) increase in option complexity.

[preyunk]

Willing to take up this issue, would be great if I could get some guidance.

[rexagod]

@preyunk I was thinking about working on this, but if you're interested, go ahead! Post your doubts regarding build/ci/code/etc and I'll clarify those for you. ✌️

[preyunk]

@rexagod I explored the file structure and according to me the related code must be in lib/http.js and lib/internal/http2/util.js or lib/internal/http2/core.js right?

[preyunk]

Also please help me with the part of code which needs to be changed or where the code needs to be added.

[rexagod]

@preyunk To clarify, maxHeaderSize will be an "alias" for maxHeaderListSize and its addition will contribute towards maintaining a similar developer experience between HTTP/1 and HTTP/2 (by matching the API in HTTP/1(maxHeaderSize) to HTTP/2(maxHeaderListSize)) . The code you should be looking for (maxHeaderListSize) can be found in util.js and core.js.

[rexagod]

maxHeaderSize: https://nodejs.org/docs/latest-v14.x/api/http.html#http_http_maxheadersize
maxHeaderListSize: https://nodejs.org/docs/latest-v14.x/api/http2.html#http2_settings_object

[preyunk]

@rexagod so I made changes in util.js and core.js adding an option maxHeaderSize which is same as maxHeaderListSize.

[preyunk]

@rexagod can you check if I am doing the right thing?

[rexagod]

Seeing that you are trying to make your first contribution here, you can use this diff as a reference. Send a draft PR here, after incorporating these changes. We'll add tests then, and/or change existing ones accordingly. Also, please ask here if there's anything you don't understand.

diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index 8a00cb65e9..40f54e9677 100644
--- a/lib/internal/http2/core.js
+++ b/lib/internal/http2/core.js
@@ -906,6 +906,9 @@ const validateSettings = hideStackFrames((settings) => {
   assertWithinRange('maxHeaderListSize',
                     settings.maxHeaderListSize,
                     0, kMaxInt);
+  assertWithinRange('maxHeaderSize',
+                    settings.maxHeaderSize,
+                    0, kMaxInt);
   if (settings.enablePush !== undefined &&
       typeof settings.enablePush !== 'boolean') {
     throw new ERR_HTTP2_INVALID_SETTING_VALUE('enablePush',
@@ -3112,7 +3115,7 @@ function getUnpackedSettings(buf, options = {}) {
         settings.maxFrameSize = value;
         break;
       case NGHTTP2_SETTINGS_MAX_HEADER_LIST_SIZE:
-        settings.maxHeaderListSize = value;
+        settings.maxHeaderListSize = settings.maxHeaderSize = value;
         break;
       case NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL:
         settings.enableConnectProtocol = value !== 0;
diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js
index dcc1355a32..d3852a031d 100644
--- a/lib/internal/http2/util.js
+++ b/lib/internal/http2/util.js
@@ -294,7 +294,7 @@ function getDefaultSettings() {
 
   if ((flags & (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) ===
       (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) {
-    holder.maxHeaderListSize =
+    holder.maxHeaderListSize = holder.maxHeadersSize =
       settingsBuffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE];
   }
 
@@ -322,6 +322,7 @@ function getSettings(session, remote) {
     maxFrameSize: settingsBuffer[IDX_SETTINGS_MAX_FRAME_SIZE],
     maxConcurrentStreams: settingsBuffer[IDX_SETTINGS_MAX_CONCURRENT_STREAMS],
     maxHeaderListSize: settingsBuffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE],
+    maxHeaderSize: settingsBuffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE],
     enableConnectProtocol:
       !!settingsBuffer[IDX_SETTINGS_ENABLE_CONNECT_PROTOCOL]
   };
@@ -349,10 +350,13 @@ function updateSettingsBuffer(settings) {
     settingsBuffer[IDX_SETTINGS_MAX_FRAME_SIZE] =
       settings.maxFrameSize;
   }
-  if (typeof settings.maxHeaderListSize === 'number') {
+  if (typeof settings.maxHeaderListSize === 'number' ||
+      typeof settings.maxHeaderSize === 'number') {
     flags |= (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE);
+    // Throw if both are defined and not equal
+    // ie, a fitting header inside `errors.js`
+    // make a new one if no suitable one exists
     settingsBuffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE] =
-      settings.maxHeaderListSize;
+      settings.maxHeaderListSize !== undefined ?
+        settings.maxHeaderListSize : settings.maxHeaderSize;
   }
   if (typeof settings.enablePush === 'boolean') {
     flags |= (1 << IDX_SETTINGS_ENABLE_PUSH);

[preyunk]

@rexagod Looks like I was heading in the right direction but had added some redundant code previously.
I've incorporated above mentioned changes, looks like we have to add a new error when maxHeaderSize and maxHeaderListSize are not equal but both are defined.
Also I was thinking to throw ERR_HTTP2_INVALID_SETTING_VALUE error instead of creating a new one but that is a RangeError so creating a new header in errors.js would be more appropriate.

[preyunk]

Also I am unable to understand what the lines mentioned below are doing.
if ((flags & (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) === (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) &
flags |= (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE);

[rexagod]

I think ERR_HTTP2_INVALID_SETTING_VALUE should be okay to use here. It accepts four params name, actual, min, max, the last two have default args set to undefined. We only need to pass name and actual which would make more sense than creating another one for the same purpose.

Also, flags |= (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE) sets the bit at IDX_SETTINGS_MAX_HEADER_LIST_SIZEth position to 1 in flags (LLS then OR). ((flags & (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) === (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) checks if the bit at IDX_SETTINGS_MAX_HEADER_LIST_SIZEth position in flags that's set (1) or unset(0) is equal to shifting 1 to that position. We are basically setting bits in flags that will help us later to get more info about different settings and their values (eg setupHandle function in core.js).

[preyunk]

@rexagod I must write tests for this in test-http2-getpackedsettings.js right?
Also how are we able to pass packed settings with those hex values in test-http2-getpackedsettings.js .

[rexagod]

Actually, for now, just add maxHeaderSize wherever maxHeaderListSize tests are present so we know its working as expected. We can always add more tests later specifically for maxHeaderSize if any reviewer asks for it. Also, documentation should be added for the same.

The buffer we pass into getUnpackedSettings is read in a Big Endian style skipping and reading 16*offset bits at a time (that's why offset skips twice at every iteration till it reaches buffer's length), which gives us our id, similarly for value except they are calculated using 32 bits BE style. Then the constant that matches the id is assigned this value. Imagine this as a "rotating" read/write ((buf.length % 6 !== 0)) that reads the area of the buffer it is in (id), calculates its value, and assigns that to its matching constant.

Let's do any further discussions on the PR itself since it's polluting the issue here.