From df33d8d0b39be893ad7864cc5e1b65bc448ba1f4 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sat, 25 Nov 2017 12:44:07 -0800 Subject: [PATCH] http2: reduce code duplication in settings PR-URL: https://github.com/nodejs/node/pull/17328 Fixes: https://github.com/nodejs/node/issues/15303 Reviewed-By: Anatoli Papirovski Reviewed-By: Sebastiaan Deckers --- lib/internal/http2/core.js | 106 ++++++------------ test/parallel/test-http2-getpackedsettings.js | 10 +- 2 files changed, 35 insertions(+), 81 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 5a28ee7492a..4225c9c2439 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -661,6 +661,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 { @@ -844,29 +871,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); @@ -2364,30 +2369,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(); } @@ -2398,7 +2380,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); @@ -2409,7 +2391,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; @@ -2427,30 +2409,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 index 7461176c5fc..16c84913893 100644 --- 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