fix(ClientRequest): Use native abort and destroy (#2000)

- Stop monkey patching `abort` on the overridden `ClientRequest`.
- Stop prematurely associating the socket with the request.
- Prefer `destroy` instead of emitting an error when appropriate.

lib/intercepted_request_router.js

@@ -13,6 +13,20 @@ const globalEmitter = require('./global_emitter')
 const Socket = require('./socket')
 const { playbackInterceptor } = require('./playback_interceptor')
 
+function socketOnClose(req) {
+  debug('socket close')
+
+  if (!req.res && !req.socket._hadError) {
+    // If we don't have a response then we know that the socket
+    // ended prematurely and we need to emit an error on the request.
+    req.socket._hadError = true
+    const err = new Error('socket hang up')
+    err.code = 'ECONNRESET'
+    req.emit('error', err)
+  }
+  req.emit('close')
+}
+
 /**
  * Given a group of interceptors, appropriately route an outgoing request.
  * Identify which interceptor ought to respond, if any, then delegate to
@@ -48,10 +62,15 @@ class InterceptedRequestRouter {
     this.readyToStartPlaybackOnSocketEvent = false
 
     this.attachToReq()
+
+    // Emit a fake socket event on the next tick to mimic what would happen on a real request.
+    // Some clients listen for a 'socket' event to be emitted before calling end(),
+    // which causes Nock to hang.
+    process.nextTick(() => this.connectSocket())
   }
 
   attachToReq() {
-    const { req, socket, options } = this
+    const { req, options } = this
 
     for (const [name, val] of Object.entries(options.headers)) {
       req.setHeader(name.toLowerCase(), val)
@@ -68,18 +87,9 @@ class InterceptedRequestRouter {
     req.path = options.path
     req.method = options.method
 
-    // ClientRequest.connection is an alias for ClientRequest.socket
-    // https://nodejs.org/api/http.html#http_request_socket
-    // https://github.com/nodejs/node/blob/b0f75818f39ed4e6bd80eb7c4010c1daf5823ef7/lib/_http_client.js#L640-L641
-    // The same Socket is shared between the request and response to mimic native behavior.
-    req.socket = req.connection = socket
-
-    propagate(['close', 'error', 'timeout'], req.socket, req)
-
     req.write = (...args) => this.handleWrite(...args)
     req.end = (...args) => this.handleEnd(...args)
     req.flushHeaders = (...args) => this.handleFlushHeaders(...args)
-    req.abort = (...args) => this.handleAbort(...args)
 
     // https://github.com/nock/nock/issues/256
     if (options.headers.expect === '100-continue') {
@@ -88,57 +98,59 @@ class InterceptedRequestRouter {
         req.emit('continue')
       })
     }
+  }
 
-    // Emit a fake socket event on the next tick to mimic what would happen on a real request.
-    // Some clients listen for a 'socket' event to be emitted before calling end(),
-    // which causes nock to hang.
-    process.nextTick(() => {
-      if (req.aborted) {
-        return
-      }
+  connectSocket() {
+    const { req, socket } = this
 
-      socket.connecting = false
-      req.emit('socket', socket)
+    // Until Node 14 is the minimum, we need to look at both flags to see if the request has been cancelled.
+    if (req.destroyed || req.aborted) {
+      return
+    }
 
-      // https://nodejs.org/api/net.html#net_event_connect
-      socket.emit('connect')
+    // ClientRequest.connection is an alias for ClientRequest.socket
+    // https://nodejs.org/api/http.html#http_request_socket
+    // https://github.com/nodejs/node/blob/b0f75818f39ed4e6bd80eb7c4010c1daf5823ef7/lib/_http_client.js#L640-L641
+    // The same Socket is shared between the request and response to mimic native behavior.
+    req.socket = req.connection = socket
 
-      // https://nodejs.org/api/tls.html#tls_event_secureconnect
-      if (socket.authorized) {
-        socket.emit('secureConnect')
-      }
+    propagate(['error', 'timeout'], socket, req)
+    socket.on('close', () => socketOnClose(req))
 
-      if (this.readyToStartPlaybackOnSocketEvent) {
-        this.maybeStartPlayback()
-      }
-    })
-  }
+    socket.connecting = false
+    req.emit('socket', socket)
 
-  emitError(error) {
-    const { req } = this
-    process.nextTick(() => {
-      req.emit('error', error)
-    })
+    // https://nodejs.org/api/net.html#net_event_connect
+    socket.emit('connect')
+
+    // https://nodejs.org/api/tls.html#tls_event_secureconnect
+    if (socket.authorized) {
+      socket.emit('secureConnect')
+    }
+
+    if (this.readyToStartPlaybackOnSocketEvent) {
+      this.maybeStartPlayback()
+    }
   }
 
   // from docs: When write function is called with empty string or buffer, it does nothing and waits for more input.
   // However, actually implementation checks the state of finished and aborted before checking if the first arg is empty.
   handleWrite(buffer, encoding, callback) {
-    debug('write', arguments)
+    debug('request write')
     const { req } = this
 
     if (req.finished) {
       const err = new Error('write after end')
       err.code = 'ERR_STREAM_WRITE_AFTER_END'
-      this.emitError(err)
+      process.nextTick(() => req.emit('error', err))
 
       // It seems odd to return `true` here, not sure why you'd want to have
       // the stream potentially written to more, but it's what Node does.
       // https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js#L662-L665
       return true
     }
 
-    if (req.aborted) {
+    if (req.socket && req.socket.destroyed) {
       return false
     }
 
@@ -167,7 +179,7 @@ class InterceptedRequestRouter {
   }
 
   handleEnd(chunk, encoding, callback) {
-    debug('req.end')
+    debug('request end')
     const { req } = this
 
     // handle the different overloaded arg signatures
@@ -191,42 +203,10 @@ class InterceptedRequestRouter {
   }
 
   handleFlushHeaders() {
-    debug('req.flushHeaders')
+    debug('request flushHeaders')
     this.maybeStartPlayback()
   }
 
-  handleAbort() {
-    debug('req.abort')
-    const { req, socket } = this
-
-    if (req.aborted) {
-      return
-    }
-
-    // Historically, `aborted` was undefined or a timestamp.
-    // Node changed this behavior in v11 to always be a bool.
-    req.aborted = true
-    req.destroyed = true
-
-    // the order of these next three steps is important to match order of events Node would emit.
-    process.nextTick(() => req.emit('abort'))
-
-    if (!socket.connecting) {
-      if (!req.res) {
-        // If we don't have a response then we know that the socket
-        // ended prematurely and we need to emit an error on the request.
-        // Node doesn't actually emit this during the `abort` method.
-        // Instead it listens to the socket's `end` and `close` events, however,
-        // Nock's socket doesn't have all the complexities and events to
-        // replicated that directly. This is an easy way to achieve the same behavior.
-        const connResetError = new Error('socket hang up')
-        connResetError.code = 'ECONNRESET'
-        this.emitError(connResetError)
-      }
-      socket.destroy()
-    }
-  }
-
   /**
    * Set request headers of the given request. This is needed both during the
    * routing phase, in case header filters were specified, and during the
@@ -268,7 +248,8 @@ class InterceptedRequestRouter {
       return
     }
 
-    if (!req.aborted && !playbackStarted) {
+    // Until Node 14 is the minimum, we need to look at both flags to see if the request has been cancelled.
+    if (!req.destroyed && !req.aborted && !playbackStarted) {
       this.startPlayback()
     }
   }
@@ -345,14 +326,11 @@ class InterceptedRequestRouter {
         // We send the raw buffer as we received it, not as we interpreted it.
         newReq.end(requestBodyBuffer)
       } else {
-        const err = new Error(
-          `Nock: No match for request ${common.stringifyRequest(
-            options,
-            requestBodyString
-          )}`
-        )
+        const reqStr = common.stringifyRequest(options, requestBodyString)
+        const err = new Error(`Nock: No match for request ${reqStr}`)
+        err.code = 'ERR_NOCK_NO_MATCH'
         err.statusCode = err.status = 404
-        this.emitError(err)
+        req.destroy(err)
       }
     }
   }

lib/playback_interceptor.js

@@ -123,12 +123,6 @@ function playbackInterceptor({
 }) {
   const { logger } = interceptor.scope
 
-  function emitError(error) {
-    process.nextTick(() => {
-      req.emit('error', error)
-    })
-  }
-
   function start() {
     interceptor.markConsumed()
     interceptor.req = req
@@ -145,7 +139,7 @@ function playbackInterceptor({
       }
 
       const delay = interceptor.delayBodyInMs + interceptor.delayConnectionInMs
-      common.setTimeout(emitError, delay, error)
+      common.setTimeout(() => req.destroy(error), delay)
       return
     }
 
@@ -170,7 +164,7 @@ function playbackInterceptor({
       // wrapping in `Promise.resolve` makes it into a promise either way.
       Promise.resolve(fn.call(interceptor, options.path, parsedRequestBody))
         .then(continueWithResponseBody)
-        .catch(emitError)
+        .catch(err => req.destroy(err))
       return
     }
 
@@ -184,7 +178,7 @@ function playbackInterceptor({
 
       Promise.resolve(fn.call(interceptor, options.path, parsedRequestBody))
         .then(continueWithFullResponse)
-        .catch(emitError)
+        .catch(err => req.destroy(err))
       return
     }
 
@@ -234,7 +228,7 @@ function playbackInterceptor({
     try {
       responseBody = parseFullReplyResult(response, fullReplyResult)
     } catch (err) {
-      emitError(err)
+      req.destroy(err)
       return
     }
 

lib/socket.js

@@ -20,6 +20,9 @@ module.exports = class Socket extends EventEmitter {
     this.destroyed = false
     this.connecting = true
 
+    // Undocumented flag used by ClientRequest to ensure errors aren't double-fired
+    this._hadError = false
+
     // Maximum allowed delay. 0 means unlimited.
     this.timeout = 0
 
@@ -83,11 +86,13 @@ module.exports = class Socket extends EventEmitter {
       return this
     }
 
+    debug('socket destroy')
     this.destroyed = true
     this.readable = this.writable = false
 
     process.nextTick(() => {
       if (err) {
+        this._hadError = true
         this.emit('error', err)
       }
       this.emit('close')

tests/test_request_overrider.js

@@ -554,6 +554,7 @@ describe('Request Overrider', () => {
     nock('http://example.test').get('/').reply(200, 'hey')
 
     const req = http.get('http://example.test')
+    req.on('error', () => {}) // listen for error so it doesn't bubble
     req.once('socket', socket => {
       socket.destroy()
       done()
@@ -564,6 +565,7 @@ describe('Request Overrider', () => {
     nock('http://example.test').get('/').reply(200, 'hey')
 
     const req = http.get('http://example.test')
+    req.on('error', () => {}) // listen for error so it doesn't bubble
     req.once('socket', socket => {
       const closeSpy = sinon.spy()
       socket.on('close', closeSpy)