Added exception handlers when removing listeners from http2 sockets (#95

)

lib/http2/request.js

@@ -38,6 +38,11 @@ class Http2Request extends EventEmitter {
   constructor (options) {
     super()
     this.onError = this.onError.bind(this)
+    this.onDrain = this.onDrain.bind(this)
+    this.onClose = this.onClose.bind(this)
+    this.onResponse = this.onResponse.bind(this)
+    this.onEnd = this.onEnd.bind(this)
+
     this.registerListeners = this.registerListeners.bind(this)
     this._flushHeaders = this._flushHeaders.bind(this)
     this[kHeadersFlushed] = false
@@ -92,32 +97,45 @@ class Http2Request extends EventEmitter {
   }
 
   registerListeners () {
-    this.stream.on('drain', () => this.emit('drain', arguments))
-    this.stream.on('error', (e) => this.emit('error', e))
+    this.stream.on('drain', this.onDrain)
+    this.stream.on('error', this.onError)
+    this.stream.on('close', this.onClose)
+    this.stream.on('response', this.onResponse)
+    this.stream.on('end', this.onEnd)
+  }
 
-    this.stream.on('close', (...args) => {
-      if (this.stream.rstCode) {
-        // Emit error message in case of abnormal stream closure
-        this.onError(new Error(`HTTP/2 Stream closed with error code ${rstErrorCodesMap[this.stream.rstCode]}`))
-      }
+  onDrain (...args) {
+    this.emit('drain', ...args)
+  }
 
-      this.emit('close', args)
-      this._client.off('error', this.onError)
-      this.stream.removeAllListeners()
-      this.removeAllListeners()
-    })
+  onError (e) {
+    this.emit('error', e)
+  }
 
-    this.stream.on('response', (response) => {
-      this.emit('response', new ResponseProxy(response, this.stream))
-    })
+  onResponse (response) {
+    this.emit('response', new ResponseProxy(response, this.stream))
+  }
 
-    this.stream.on('end', () => {
-      this.emit('end')
-    })
+  onEnd () {
+    this.emit('end')
   }
 
-  onError (e) {
-    this.emit('error', e)
+  onClose (...args) {
+    if (this.stream.rstCode) {
+      // Emit error message in case of abnormal stream closure
+      this.onError(new Error(`HTTP/2 Stream closed with error code ${rstErrorCodesMap[this.stream.rstCode]}`))
+    }
+
+    this.emit('close', ...args)
+
+    this._client.off('error', this.onError)
+    this.stream.off('drain', this.onDrain)
+    this.stream.off('error', this.onError)
+    this.stream.off('response', this.onResponse)
+    this.stream.off('end', this.onEnd)
+    this.stream.off('close', this.onClose)
+
+    this.removeAllListeners()
   }
 
   setDefaultEncoding (encoding) {

request.js

@@ -1139,8 +1139,16 @@ Request.prototype.start = function () {
 
         // clean up timing event listeners if needed on error
         self.req.once('error', function () {
-          socket.removeListener('lookup', onLookupTiming)
-          socket.removeListener('connect', onConnectTiming)
+          // Swallow ERR_HTTP2_SOCKET_UNBOUND error when removing listeners in case of error.
+          // This needs to be done since http2 ClientSession disassociates the underlying socket from the session before emitting the error event
+          try {
+            socket.removeListener('lookup', onLookupTiming)
+            socket.removeListener('connect', onConnectTiming)
+          } catch (err) {
+            if (err.code !== 'ERR_HTTP2_SOCKET_UNBOUND') {
+              throw err
+            }
+          }
         })
       }
     }
@@ -1176,7 +1184,15 @@ Request.prototype.start = function () {
         socket.on('connect', onReqSockConnect)
 
         self.req.on('error', function (err) { // eslint-disable-line handle-callback-err
-          socket.removeListener('connect', onReqSockConnect)
+          // Swallow ERR_HTTP2_SOCKET_UNBOUND error when removing listeners in case of error.
+          // This needs to be done since http2 ClientSession disassociates the underlying socket from the session before emitting the error event
+          try {
+            socket.removeListener('connect', onReqSockConnect)
+          } catch (err) {
+            if (err.code !== 'ERR_HTTP2_SOCKET_UNBOUND') {
+              throw err
+            }
+          }
         })
 
         // Set a timeout in memory - this block will throw if the server takes more

tests/test-ssl-http2.js

@@ -26,7 +26,15 @@ var http2SecureServer = server.createHttp2Server({
   rejectUnauthorized: true
 })
 
+var httpsServer = server.createSSLServer({
+  key: path.resolve(__dirname, 'ssl/ca/localhost.key'),
+  cert: path.resolve(__dirname, 'ssl/ca/localhost.crt'),
+  ca: caPath,
+  rejectUnauthorized: true
+})
+
 destroyable(http2SecureServer)
+destroyable(httpsServer)
 
 tape('setup', function (t) {
   http2SecureServer.on('/', function (req, res) {
@@ -39,8 +47,20 @@ tape('setup', function (t) {
     }
   })
 
+  httpsServer.on('/', function (req, res) {
+    if (req.connection.authorized) {
+      res.writeHead(200, { 'Content-Type': 'text/plain' })
+      res.end('authorized')
+    } else {
+      res.writeHead(401, { 'Content-Type': 'text/plain' })
+      res.end('unauthorized')
+    }
+  })
+
   http2SecureServer.listen(0, function () {
-    t.end()
+    httpsServer.listen(0, function () {
+      t.end()
+    })
   })
 })
 
@@ -143,8 +163,26 @@ tape('ca + extraCA', function (t) {
   })
 })
 
+tape('http2 -> https', function (t) {
+  request({
+    url: httpsServer.url,
+    ca: ca,
+    key: clientKey,
+    cert: clientCert,
+    protocolVersion: 'http2'
+  }, function (err, res, body) {
+    t.notEqual(err, null)
+    t.equal(err.code, 'ERR_HTTP2_ERROR')
+    t.equal(err.errno, -505)
+
+    t.end()
+  })
+})
+
 tape('cleanup', function (t) {
   http2SecureServer.destroy(function () {
-    t.end()
+    httpsServer.destroy(function () {
+      t.end()
+    })
   })
 })