fix(router): bring behavior of abort() inline with native Node

Nocks behavior around events and errors relating to aborting and ending requests has not lined up
with Node for while. I dug into an investigation of how it differs, if there were git commits/pull-requests explaining why they differ,
and trying to answer the question of should Nock change to line up better.

The single largest deviation were the errors emitted around aborting or already aborted requests.
Emitting an error on abort was added to fix issue nock#439.
The conversation talks about how Node emits an error post abort.
> ... in practice the `http` module does emit an `ECONNRESET` error after aborting. ... - Jan 7, 2016

However, this is only true between the 'socket' and 'response' events.
Otherwise calling abort does not emit an error of any kind.

Determining exactly what Nock should do is a bit of moving target.
As Node's internals of HTTP, Net, Stream, and micro task timing have changed a lot since
iojs/v4, when Nock added these errors, and v13, current at this time.

My conclusion is that none of Nock's deviations from Node's _documented_ behavior
are user features that should be maintained. If anything, bringing Nock closer to Node
better fulfills original requests.
nock#282

The entire `test_abort.js` file was scraped and rewritten by writing assertions against Node behavior, without Nock first.
Then adding Nock into each test to ensure events and errors were correct.

BREAKING CHANGE:

Calling `request.abort()`:
- Use to always emit a 'socket hang up' error. Now only emits the error if `abort` is called between the 'socket' and 'response' events.
- The emitted 'abort' event now happens on `nextTick`.
- The socket is now only destroyed if the 'socket' event has fired, and now emits a 'close' event on `nextTick` that propagates though the request object.
- `request.aborted` attribute is set to `true` instead of a timestamp. Changed in Node v11.0 nodejs/node#20230

Calling `write`, `end`, or `flushHeaders` on an aborted request no longer emits an error.
However, writing to a request that is already finished (ended) will emit a 'write after end' error.

Playback of a mocked responses will now never happen until the 'socket' event is emitted.
The 'socket' event is still artificially set to emit on `nextTick` when a ClientRequest is created.
This means in the following code the Scope will never be done because at least one tick needs
to happen before any matched Interceptor is considered consumed.
```js
const scope = nock(...).get('/').reply()
const req = http.get(...)
scope.done()
```

Author: mastermatt

lib/intercepted_request_router.js

@@ -40,15 +40,14 @@ class InterceptedRequestRouter {
 
     this.response = new IncomingMessage(this.socket)
     this.playbackStarted = false
+    this.playbackOnSocket = false
     this.requestBodyBuffers = []
 
     this.attachToReq()
   }
 
   attachToReq() {
-    const { req, response, socket, options } = this
-
-    response.req = req
+    const { req, socket, options } = this
 
     for (const [name, val] of Object.entries(options.headers)) {
       req.setHeader(name.toLowerCase(), val)
@@ -71,7 +70,7 @@ class InterceptedRequestRouter {
     // The same Socket is shared between the request and response to mimic native behavior.
     req.socket = req.connection = socket
 
-    propagate(['error', 'timeout'], req.socket, req)
+    propagate(['close', 'error', 'timeout'], req.socket, req)
 
     req.write = (...args) => this.handleWrite(...args)
     req.end = (...args) => this.handleEnd(...args)
@@ -90,6 +89,11 @@ class InterceptedRequestRouter {
     // Some clients listen for a 'socket' event to be emitted before calling end(),
     // which causes nock to hang.
     process.nextTick(() => {
+      if (req.aborted) {
+        return
+      }
+
+      socket.connecting = false
       req.emit('socket', socket)
 
       // https://nodejs.org/api/net.html#net_event_connect
@@ -99,6 +103,10 @@ class InterceptedRequestRouter {
       if (socket.authorized) {
         socket.emit('secureConnect')
       }
+
+      if (this.playbackOnSocket) {
+        this.maybeStartPlayback()
+      }
     })
   }
 
@@ -109,26 +117,38 @@ class InterceptedRequestRouter {
     })
   }
 
+  // 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)
     const { req } = this
 
-    if (!req.aborted) {
-      if (buffer) {
-        if (!Buffer.isBuffer(buffer)) {
-          buffer = Buffer.from(buffer, encoding)
-        }
-        this.requestBodyBuffers.push(buffer)
-      }
-      // can't use instanceof Function because some test runners
-      // run tests in vm.runInNewContext where Function is not same
-      // as that in the current context
-      // https://github.com/nock/nock/pull/1754#issuecomment-571531407
-      if (typeof callback === 'function') {
-        callback()
-      }
-    } else {
-      this.emitError(new Error('Request aborted'))
+    if (req.finished) {
+      const err = new Error('write after end')
+      err.code = 'ERR_STREAM_WRITE_AFTER_END'
+      this.emitError(err)
+      return true
+    }
+
+    if (req.aborted) {
+      return false
+    }
+
+    if (!buffer || buffer.length === 0) {
+      return true
+    }
+
+    if (!Buffer.isBuffer(buffer)) {
+      buffer = Buffer.from(buffer, encoding)
+    }
+    this.requestBodyBuffers.push(buffer)
+
+    // can't use instanceof Function because some test runners
+    // run tests in vm.runInNewContext where Function is not same
+    // as that in the current context
+    // https://github.com/nock/nock/pull/1754#issuecomment-571531407
+    if (typeof callback === 'function') {
+      callback()
     }
 
     common.setImmediate(function() {
@@ -142,7 +162,7 @@ class InterceptedRequestRouter {
     debug('req.end')
     const { req } = this
 
-    // handle the different overloaded param signatures
+    // handle the different overloaded arg signatures
     if (typeof chunk === 'function') {
       callback = chunk
       chunk = null
@@ -155,51 +175,45 @@ class InterceptedRequestRouter {
       req.once('finish', callback)
     }
 
-    if (!req.aborted && !this.playbackStarted) {
-      req.write(chunk, encoding)
-      this.startPlayback()
-    }
-    if (req.aborted) {
-      this.emitError(new Error('Request aborted'))
-    }
+    req.write(chunk, encoding)
+    req.finished = true
+    this.maybeStartPlayback()
 
     return req
   }
 
   handleFlushHeaders() {
     debug('req.flushHeaders')
-    const { req } = this
-
-    if (!req.aborted && !this.playbackStarted) {
-      this.startPlayback()
-    }
-    if (req.aborted) {
-      this.emitError(new Error('Request aborted'))
-    }
+    this.maybeStartPlayback()
   }
 
   handleAbort() {
     debug('req.abort')
-    const { req, response, socket } = this
+    const { req, socket } = this
 
     if (req.aborted) {
       return
     }
-    req.aborted = Date.now()
-    if (!this.playbackStarted) {
-      this.startPlayback()
+    req.aborted = true // as of v11 this is a bool instead of a timestamp
+    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()
     }
-    const err = new Error()
-    err.code = 'aborted'
-    response.emit('close', err)
-
-    socket.destroy()
-
-    req.emit('abort')
-
-    const connResetError = new Error('socket hang up')
-    connResetError.code = 'ECONNRESET'
-    this.emitError(connResetError)
   }
 
   /**
@@ -233,6 +247,21 @@ class InterceptedRequestRouter {
     }
   }
 
+  maybeStartPlayback() {
+    const { req, socket, playbackStarted } = this
+
+    // In order to get the events in the right order we need to delay playback
+    // if we get here before the `socket` event is emitted.
+    if (socket.connecting) {
+      this.playbackOnSocket = true
+      return
+    }
+
+    if (!req.aborted && !playbackStarted) {
+      this.startPlayback()
+    }
+  }
+
   startPlayback() {
     debug('ending')
     this.playbackStarted = true
@@ -274,7 +303,6 @@ class InterceptedRequestRouter {
 
       // wait to emit the finish event until we know for sure an Interceptor is going to playback.
       // otherwise an unmocked request might emit finish twice.
-      req.finished = true
       req.emit('finish')
 
       playbackInterceptor({

lib/playback_interceptor.js

@@ -257,10 +257,6 @@ function playbackInterceptor({
 
     interceptor.markConsumed()
 
-    if (req.aborted) {
-      return
-    }
-
     response.rawHeaders.push(
       ...selectDefaultHeaders(
         response.rawHeaders,
@@ -277,24 +273,24 @@ function playbackInterceptor({
 
     response.headers = common.headersArrayToObject(response.rawHeaders)
 
-    process.nextTick(() =>
-      respondUsingInterceptor({
-        responseBody,
-        responseBuffers,
-      })
-    )
+    respondUsingInterceptor({
+      responseBody,
+      responseBuffers,
+    })
   }
 
   function respondUsingInterceptor({ responseBody, responseBuffers }) {
-    if (req.aborted) {
-      return
-    }
-
     function respond() {
       if (req.aborted) {
         return
       }
 
+      // Even though we've had the response object for awhile at this point,
+      // we only attach it to the request immediately before the `response`
+      // event because, as in Node, it alters the error handling around aborts.
+      req.res = response
+      response.req = req
+
       debug('emitting response')
       req.emit('response', response)
 
@@ -342,7 +338,15 @@ function playbackInterceptor({
     }
   }
 
-  start()
+  // stall the playback to ensure the correct events are emitted first and
+  // any aborts in the queue can be called.
+  common.setImmediate(() => {
+    if (req.aborted) {
+      return
+    }
+
+    start()
+  })
 }
 
 module.exports = { playbackInterceptor }

lib/socket.js

@@ -18,7 +18,7 @@ module.exports = class Socket extends EventEmitter {
     this.readable = true
     this.pending = false
     this.destroyed = false
-    this.connecting = false
+    this.connecting = true
 
     // totalDelay that has already been applied to the current
     // request/connection, timeout error will be generated if
@@ -70,11 +70,20 @@ module.exports = class Socket extends EventEmitter {
   }
 
   destroy(err) {
+    if (this.destroyed) {
+      return this
+    }
+
     this.destroyed = true
     this.readable = this.writable = false
-    if (err) {
-      this.emit('error', err)
-    }
+
+    process.nextTick(() => {
+      if (err) {
+        this.emit('error', err)
+      }
+      this.emit('close')
+    })
+
     return this
   }
 }