http2: reduce code duplication in settings

jasnell · MylesBorins · commit 31cdbd636a15 · 2017-12-19T13:30:51.000-05:00
PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
@@ -659,6 +659,33 @@ function pingCallback(cb) {
   };
 }
 
+function validateSettings(settings) {
+  settings = Object.assign({}, settings);
+  assertWithinRange('headerTableSize',
+                    settings.headerTableSize,
+                    0, kMaxInt);
+  assertWithinRange('initialWindowSize',
+                    settings.initialWindowSize,
+                    0, kMaxInt);
+  assertWithinRange('maxFrameSize',
+                    settings.maxFrameSize,
+                    16384, kMaxFrameSize);
+  assertWithinRange('maxConcurrentStreams',
+                    settings.maxConcurrentStreams,
+                    0, kMaxStreams);
+  assertWithinRange('maxHeaderListSize',
+                    settings.maxHeaderListSize,
+                    0, kMaxInt);
+  if (settings.enablePush !== undefined &&
+      typeof settings.enablePush !== 'boolean') {
+    const err = new errors.TypeError('ERR_HTTP2_INVALID_SETTING_VALUE',
+                                     'enablePush', settings.enablePush);
+    err.actual = settings.enablePush;
+    throw err;
+  }
+  return settings;
+}
+
 // Upon creation, the Http2Session takes ownership of the socket. The session
 // may not be ready to use immediately if the socket is not yet fully connected.
 class Http2Session extends EventEmitter {
@@ -842,29 +869,7 @@ class Http2Session extends EventEmitter {
 
     // Validate the input first
     assertIsObject(settings, 'settings');
-    settings = Object.assign(Object.create(null), settings);
-    assertWithinRange('headerTableSize',
-                      settings.headerTableSize,
-                      0, kMaxInt);
-    assertWithinRange('initialWindowSize',
-                      settings.initialWindowSize,
-                      0, kMaxInt);
-    assertWithinRange('maxFrameSize',
-                      settings.maxFrameSize,
-                      16384, kMaxFrameSize);
-    assertWithinRange('maxConcurrentStreams',
-                      settings.maxConcurrentStreams,
-                      0, kMaxStreams);
-    assertWithinRange('maxHeaderListSize',
-                      settings.maxHeaderListSize,
-                      0, kMaxInt);
-    if (settings.enablePush !== undefined &&
-        typeof settings.enablePush !== 'boolean') {
-      const err = new errors.TypeError('ERR_HTTP2_INVALID_SETTING_VALUE',
-                                       'enablePush', settings.enablePush);
-      err.actual = settings.enablePush;
-      throw err;
-    }
+    settings = validateSettings(settings);
     if (state.pendingAck === state.maxPendingAck) {
       throw new errors.Error('ERR_HTTP2_MAX_PENDING_SETTINGS_ACK',
                              this[kState].pendingAck);
@@ -2362,30 +2367,7 @@ function createServer(options, handler) {
 // HTTP2-Settings header frame.
 function getPackedSettings(settings) {
   assertIsObject(settings, 'settings');
-  settings = settings || Object.create(null);
-  assertWithinRange('headerTableSize',
-                    settings.headerTableSize,
-                    0, kMaxInt);
-  assertWithinRange('initialWindowSize',
-                    settings.initialWindowSize,
-                    0, kMaxInt);
-  assertWithinRange('maxFrameSize',
-                    settings.maxFrameSize,
-                    16384, kMaxFrameSize);
-  assertWithinRange('maxConcurrentStreams',
-                    settings.maxConcurrentStreams,
-                    0, kMaxStreams);
-  assertWithinRange('maxHeaderListSize',
-                    settings.maxHeaderListSize,
-                    0, kMaxInt);
-  if (settings.enablePush !== undefined &&
-      typeof settings.enablePush !== 'boolean') {
-    const err = new errors.TypeError('ERR_HTTP2_INVALID_SETTING_VALUE',
-                                     'enablePush', settings.enablePush);
-    err.actual = settings.enablePush;
-    throw err;
-  }
-  updateSettingsBuffer(settings);
+  updateSettingsBuffer(validateSettings(settings));
   return binding.packSettings();
 }
 
@@ -2396,7 +2378,7 @@ function getUnpackedSettings(buf, options = {}) {
   }
   if (buf.length % 6 !== 0)
     throw new errors.RangeError('ERR_HTTP2_INVALID_PACKED_SETTINGS_LENGTH');
-  const settings = Object.create(null);
+  const settings = {};
   let offset = 0;
   while (offset < buf.length) {
     const id = buf.readUInt16BE(offset);
@@ -2407,7 +2389,7 @@ function getUnpackedSettings(buf, options = {}) {
         settings.headerTableSize = value;
         break;
       case NGHTTP2_SETTINGS_ENABLE_PUSH:
-        settings.enablePush = value;
+        settings.enablePush = value !== 0;
         break;
       case NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS:
         settings.maxConcurrentStreams = value;
@@ -2425,30 +2407,8 @@ function getUnpackedSettings(buf, options = {}) {
     offset += 4;
   }
 
-  if (options != null && options.validate) {
-    assertWithinRange('headerTableSize',
-                      settings.headerTableSize,
-                      0, kMaxInt);
-    assertWithinRange('enablePush',
-                      settings.enablePush,
-                      0, 1);
-    assertWithinRange('initialWindowSize',
-                      settings.initialWindowSize,
-                      0, kMaxInt);
-    assertWithinRange('maxFrameSize',
-                      settings.maxFrameSize,
-                      16384, kMaxFrameSize);
-    assertWithinRange('maxConcurrentStreams',
-                      settings.maxConcurrentStreams,
-                      0, kMaxStreams);
-    assertWithinRange('maxHeaderListSize',
-                      settings.maxHeaderListSize,
-                      0, kMaxInt);
-  }
-
-  if (settings.enablePush !== undefined) {
-    settings.enablePush = !!settings.enablePush;
-  }
+  if (options != null && options.validate)
+    validateSettings(settings);
 
   return settings;
 }
diff --git a/test/parallel/test-http2-getpackedsettings.js b/test/parallel/test-http2-getpackedsettings.js
@@ -128,7 +128,6 @@ assert.doesNotThrow(() => http2.getPackedSettings({ enablePush: false }));
   assert.strictEqual(settings.enablePush, true);
 }
 
-//should throw if enablePush is not 0 or 1
 {
   const packed = Buffer.from([
     0x00, 0x02, 0x00, 0x00, 0x00, 0x00]);
@@ -140,13 +139,8 @@ assert.doesNotThrow(() => http2.getPackedSettings({ enablePush: false }));
   const packed = Buffer.from([
     0x00, 0x02, 0x00, 0x00, 0x00, 0x64]);
 
-  assert.throws(() => {
-    http2.getUnpackedSettings(packed, { validate: true });
-  }, common.expectsError({
-    code: 'ERR_HTTP2_INVALID_SETTING_VALUE',
-    type: RangeError,
-    message: 'Invalid value for setting "enablePush": 100'
-  }));
+  const settings = http2.getUnpackedSettings(packed, { validate: true });
+  assert.strictEqual(settings.enablePush, true);
 }
 
 //check for what happens if passing {validate: true} and no errors happen
