http2: multiple cleanups and s/streamClosed/close

doc/api/http2.md

@@ -633,7 +633,7 @@ All [`Http2Stream`][] instances are destroyed either when:
 When an `Http2Stream` instance is destroyed, an attempt will be made to send an
 `RST_STREAM` frame will be sent to the connected peer.
 
-Once the `Http2Stream` instance is destroyed, the `'streamClosed'` event will
+When the `Http2Stream` instance is destroyed, the `'close'` event will
 be emitted. Because `Http2Stream` is an instance of `stream.Duplex`, the
 `'end'` event will also be emitted if the stream data is currently flowing.
 The `'error'` event may also be emitted if `http2stream.destroy()` was called
@@ -655,6 +655,18 @@ abnormally aborted in mid-communication.
 *Note*: The `'aborted'` event will only be emitted if the `Http2Stream`
 writable side has not been ended.
 
+#### Event: 'close'
+<!-- YAML
+added: v8.4.0
+-->
+
+The `'close'` event is emitted when the `Http2Stream` is destroyed. Once
+this event is emitted, the `Http2Stream` instance is no longer usable.
+
+The listener callback is passed a single argument specifying the HTTP/2 error
+code specified when closing the stream. If the code is any value other than
+`NGHTTP2_NO_ERROR` (`0`), an `'error'` event will also be emitted.
+
 #### Event: 'error'
 <!-- YAML
 added: v8.4.0
@@ -674,18 +686,6 @@ argument identifying the frame type, and an integer argument identifying the
 error code. The `Http2Stream` instance will be destroyed immediately after the
 `'frameError'` event is emitted.
 
-#### Event: 'streamClosed'
-<!-- YAML
-added: v8.4.0
--->
-
-The `'streamClosed'` event is emitted when the `Http2Stream` is destroyed. Once
-this event is emitted, the `Http2Stream` instance is no longer usable.
-
-The listener callback is passed a single argument specifying the HTTP/2 error
-code specified when closing the stream. If the code is any value other than
-`NGHTTP2_NO_ERROR` (`0`), an `'error'` event will also be emitted.
-
 #### Event: 'timeout'
 <!-- YAML
 added: v8.4.0

lib/internal/http2/compat.js

@@ -122,20 +122,6 @@ function onStreamDrain() {
     response.emit('drain');
 }
 
-// TODO Http2Stream does not emit 'close'
-function onStreamClosedRequest() {
-  const request = this[kRequest];
-  if (request !== undefined)
-    request.push(null);
-}
-
-// TODO Http2Stream does not emit 'close'
-function onStreamClosedResponse() {
-  const response = this[kResponse];
-  if (response !== undefined)
-    response.emit('finish');
-}
-
 function onStreamAbortedRequest() {
   const request = this[kRequest];
   if (request !== undefined && request[kState].closed === false) {
@@ -247,10 +233,9 @@ class Http2ServerRequest extends Readable {
     stream.on('trailers', onStreamTrailers);
     stream.on('end', onStreamEnd);
     stream.on('error', onStreamError);
-    stream.on('close', onStreamClosedRequest);
     stream.on('aborted', onStreamAbortedRequest);
     const onfinish = this[kFinish].bind(this);
-    stream.on('streamClosed', onfinish);
+    stream.on('close', onfinish);
     stream.on('finish', onfinish);
     this.on('pause', onRequestPause);
     this.on('resume', onRequestResume);
@@ -380,10 +365,9 @@ class Http2ServerResponse extends Stream {
     stream[kResponse] = this;
     this.writable = true;
     stream.on('drain', onStreamDrain);
-    stream.on('close', onStreamClosedResponse);
     stream.on('aborted', onStreamAbortedResponse);
     const onfinish = this[kFinish].bind(this);
-    stream.on('streamClosed', onfinish);
+    stream.on('close', onfinish);
     stream.on('finish', onfinish);
   }
 

lib/internal/http2/core.js

@@ -28,6 +28,8 @@ const { _connectionListener: httpConnectionListener } = require('http');
 const { createPromise, promiseResolve } = process.binding('util');
 const debug = util.debuglog('http2');
 
+const kMaxFrameSize = (2 ** 24) - 1;
+const kMaxInt = (2 ** 32) - 1;
 const kMaxStreams = (2 ** 31) - 1;
 
 const {
@@ -224,10 +226,8 @@ function onStreamTrailers() {
   return headersList;
 }
 
-// Called when the stream is closed. The streamClosed event is emitted on the
-// Http2Stream instance. Note that this event is distinctly different than the
-// require('stream') interface 'close' event which deals with the state of the
-// Readable and Writable sides of the Duplex.
+// Called when the stream is closed. The close event is emitted on the
+// Http2Stream instance
 function onStreamClose(code) {
   const stream = this[kOwner];
   stream[kUpdateTimer]();
@@ -332,9 +332,9 @@ function emitGoaway(self, code, lastStreamID, buf) {
     return;
   if (!state.shuttingDown && !state.shutdown) {
     self.shutdown({}, self.destroy.bind(self));
-  } else {
-    self.destroy();
+    return;
   }
+  self.destroy();
 }
 
 // Called by the native layer when a goaway frame has been received
@@ -582,14 +582,15 @@ function doShutdown(options) {
 function submitShutdown(options) {
   const type = this[kType];
   debug(`Http2Session ${sessionName(type)}: submitting shutdown request`);
+  const shutdownFn = doShutdown.bind(this, options);
   if (type === NGHTTP2_SESSION_SERVER && options.graceful === true) {
     // first send a shutdown notice
     this[kHandle].shutdownNotice();
     // then, on flip of the event loop, do the actual shutdown
-    setImmediate(doShutdown.bind(this), options);
-  } else {
-    doShutdown.call(this, options);
+    setImmediate(shutdownFn);
+    return;
   }
+  shutdownFn();
 }
 
 function finishSessionDestroy(socket) {
@@ -658,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 {
@@ -707,7 +735,9 @@ class Http2Session extends EventEmitter {
     const setupFn = setupHandle(this, socket, type, options);
     if (socket.connecting) {
       this[kState].connecting = true;
-      socket.once('connect', setupFn);
+      const connectEvent =
+        socket instanceof tls.TLSSocket ? 'secureConnect' : 'connect';
+      socket.once(connectEvent, setupFn);
     } else {
       setupFn();
     }
@@ -839,41 +869,20 @@ class Http2Session extends EventEmitter {
 
     // Validate the input first
     assertIsObject(settings, 'settings');
-    settings = Object.assign(Object.create(null), settings);
-    assertWithinRange('headerTableSize',
-                      settings.headerTableSize,
-                      0, 2 ** 32 - 1);
-    assertWithinRange('initialWindowSize',
-                      settings.initialWindowSize,
-                      0, 2 ** 32 - 1);
-    assertWithinRange('maxFrameSize',
-                      settings.maxFrameSize,
-                      16384, 2 ** 24 - 1);
-    assertWithinRange('maxConcurrentStreams',
-                      settings.maxConcurrentStreams,
-                      0, kMaxStreams);
-    assertWithinRange('maxHeaderListSize',
-                      settings.maxHeaderListSize,
-                      0, 2 ** 32 - 1);
-    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);
     }
     debug(`Http2Session ${sessionName(this[kType])}: sending settings`);
 
     state.pendingAck++;
+    const settingsFn = submitSettings.bind(this, settings);
     if (state.connecting) {
-      this.once('connect', submitSettings.bind(this, settings));
+      this.once('connect', settingsFn);
       return;
     }
-    submitSettings.call(this, settings);
+    settingsFn();
   }
 
   // Destroy the Http2Session
@@ -959,13 +968,14 @@ class Http2Session extends EventEmitter {
       this.on('shutdown', callback);
     }
 
+    const shutdownFn = submitShutdown.bind(this, options);
     if (state.connecting) {
-      this.once('connect', submitShutdown.bind(this, options));
+      this.once('connect', shutdownFn);
       return;
     }
 
     debug(`Http2Session ${sessionName(type)}: sending shutdown`);
-    submitShutdown.call(this, options);
+    shutdownFn();
   }
 
   _onTimeout() {
@@ -1366,15 +1376,15 @@ class Http2Stream extends Duplex {
   rstStream(code = NGHTTP2_NO_ERROR) {
     if (typeof code !== 'number')
       throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'code', 'number');
-    if (code < 0 || code > 2 ** 32 - 1)
+    if (code < 0 || code > kMaxInt)
       throw new errors.RangeError('ERR_OUT_OF_RANGE', 'code');
 
-    const fn = submitRstStream.bind(this, code);
+    const rstStreamFn = submitRstStream.bind(this, code);
     if (this[kID] === undefined) {
-      this.once('ready', fn);
+      this.once('ready', rstStreamFn);
       return;
     }
-    fn();
+    rstStreamFn();
   }
 
   rstWithNoError() {
@@ -1405,12 +1415,12 @@ class Http2Stream extends Duplex {
     options = Object.assign({}, options);
     validatePriorityOptions(options);
 
-    const fn = submitPriority.bind(this, options);
+    const priorityFn = submitPriority.bind(this, options);
     if (this[kID] === undefined) {
-      this.once('ready', fn);
+      this.once('ready', priorityFn);
       return;
     }
-    fn();
+    priorityFn();
   }
 
   // Called by this.destroy().
@@ -1473,7 +1483,7 @@ function continueStreamDestroy(err, callback) {
   abort(this);
   this.push(null);  // Close the readable side
   this.end();       // Close the writable side
-  process.nextTick(emit, this, 'streamClosed', code);
+  process.nextTick(emit, this, 'close', code);
 }
 
 function finishStreamDestroy() {
@@ -2357,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, 2 ** 32 - 1);
-  assertWithinRange('initialWindowSize',
-                    settings.initialWindowSize,
-                    0, 2 ** 32 - 1);
-  assertWithinRange('maxFrameSize',
-                    settings.maxFrameSize,
-                    16384, 2 ** 24 - 1);
-  assertWithinRange('maxConcurrentStreams',
-                    settings.maxConcurrentStreams,
-                    0, kMaxStreams);
-  assertWithinRange('maxHeaderListSize',
-                    settings.maxHeaderListSize,
-                    0, 2 ** 32 - 1);
-  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();
 }
 
@@ -2391,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);
@@ -2402,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;
@@ -2420,30 +2407,8 @@ function getUnpackedSettings(buf, options = {}) {
     offset += 4;
   }
 
-  if (options != null && options.validate) {
-    assertWithinRange('headerTableSize',
-                      settings.headerTableSize,
-                      0, 2 ** 32 - 1);
-    assertWithinRange('enablePush',
-                      settings.enablePush,
-                      0, 1);
-    assertWithinRange('initialWindowSize',
-                      settings.initialWindowSize,
-                      0, 2 ** 32 - 1);
-    assertWithinRange('maxFrameSize',
-                      settings.maxFrameSize,
-                      16384, 2 ** 24 - 1);
-    assertWithinRange('maxConcurrentStreams',
-                      settings.maxConcurrentStreams,
-                      0, kMaxStreams);
-    assertWithinRange('maxHeaderListSize',
-                      settings.maxHeaderListSize,
-                      0, 2 ** 32 - 1);
-  }
-
-  if (settings.enablePush !== undefined) {
-    settings.enablePush = !!settings.enablePush;
-  }
+  if (options != null && options.validate)
+    validateSettings(settings);
 
   return settings;
 }

test/parallel/test-http2-client-http1-server.js

@@ -13,7 +13,7 @@ server.listen(0, common.mustCall(() => {
   const client = http2.connect(`http://localhost:${server.address().port}`);
 
   const req = client.request();
-  req.on('streamClosed', common.mustCall());
+  req.on('close', common.mustCall());
 
   client.on('error', common.expectsError({
     code: 'ERR_HTTP2_ERROR',

test/parallel/test-http2-client-rststream-before-connect.js

@@ -27,7 +27,7 @@ server.on('listening', common.mustCall(() => {
   // second call doesn't do anything
   assert.doesNotThrow(() => req.rstStream(8));
 
-  req.on('streamClosed', common.mustCall((code) => {
+  req.on('close', common.mustCall((code) => {
     assert.strictEqual(req.destroyed, true);
     assert.strictEqual(code, 0);
     server.close();