feat: remove socketDelay (#1974)

From a user's perspective, having both `delayConnection` and `socketDelay` is confusing and unnecessarily. The differences between the two are subtle, and in my experience, users need to be fairly well versed in both the Node HTTP module and the internals of Nock in order to accurately determine which to use. On the surface there seems to be two use cases: - A user wants to test how their code handles a `timeout` event. Whether their client is expected to abort the request, or some other action is taken in response to the event. Either way, the user doesn't care if wall-clock time passes, and would prefer the timeout be simulated so their test suite can get on with it. - A user wants to force wall-clock time to pass before a `response` event. This is usually to test some timeout feature that is not based on the `timeout` event, or to give other code time to complete a task first. Based on those two use cases, it seems obvious that there should be two different delay functions, like we seem to have now. However, there are two subtle aspects that blur the line. - When a socket emits a `timeout` event, which the request propagates, the request does not end. This is true in Node and in Nock today. Clients my choose to abort a request upon a timeout event, they may not. - In Nock today, when the socket is "applying" the artificial timeout, to determine if it should emit a `timeout`, it doesn't just use the value passed to `socketDelay`. It uses the sum of the values passed to `delayConnection` and `socketDelay`. This covers the flow of a user wanting to ensure their code sees a `timeout` event even if no action is taken. Therefore, there is no reason to have two different options for users. The value passed to `delayConnection` can trigger a `timeout` event right away and if the code chooses not to act on the event, then it will wait in real time until `response` is triggered. In fact, when I began working on this, I went into the `test_socket_delay.js` file and replaced all `socketDelay` with `delayConnection`. All the tests passed. Other minor tweaks included in this change: - The delay methods on the Interceptor are now just setters instead of additive. This was undocumented, unintuitive behavior. - Fixed a bug from #1973, where `replayWithError` would cause Interceptors to be consumed twice. BREAKING CHANGE: - `socketDelay` has been removed. Use `delayConnection` instead. - `delay`, `delayConnection`, and `delayBody` are now setters instead of additive. example: ```js nock('http://example.com') .get('/') .delay(1) .delay({ head: 2, body: 3 }) .delayConnection(4) .delayBody(5) .delayBody(6) .reply() ``` Previously, the connection would have been delayed by 7 and the body delayed by 14. Now, the connection will be delayed by 4 and the body delayed by 6.
nock · May 2, 2020 · 6a5a3cf · 6a5a3cf
1 parent ec75f64
commit 6a5a3cf
Show file tree

Hide file tree

Showing 12 changed files with 166 additions and 222 deletions.
diff --git a/README.md b/README.md
@@ -43,10 +43,7 @@ For instance, if a module performs HTTP requests to a CouchDB server or makes HT
   - [Support for HTTP and HTTPS](#support-for-http-and-https)
   - [Non-standard ports](#non-standard-ports)
   - [Repeat response n times](#repeat-response-n-times)
-  - [Delay the response body](#delay-the-response-body)
   - [Delay the response](#delay-the-response)
-  - [Delay the connection](#delay-the-connection)
-  - [Socket timeout](#socket-timeout)
   - [Chaining](#chaining)
   - [Scope filtering](#scope-filtering)
   - [Conditional scope filtering](#conditional-scope-filtering)
@@ -660,22 +657,9 @@ nock('http://zombo.com').get('/').thrice().reply(200, 'Ok')
 
 To repeat this response for as long as nock is active, use [.persist()](#persist).
 
-### Delay the response body
-
-You are able to specify the number of milliseconds that the response body should be delayed. Response header will be replied immediately.
-`delayBody(1000)` is equivalent to `delay({body: 1000})`.
-
-```js
-nock('http://my.server.com')
-  .get('/')
-  .delayBody(2000) // 2 seconds
-  .reply(200, '<html></html>')
-```
-
-NOTE: the [`'response'`](http://nodejs.org/api/http.html#http_event_response) event will occur immediately, but the [IncomingMessage](http://nodejs.org/api/http.html#http_http_incomingmessage) will not emit its `'end'` event until after the delay.
-
 ### Delay the response
 
+Nock can simulate response latency to allow you to test timeouts, race conditions, an other timing related scenarios.  
 You are able to specify the number of milliseconds that your reply should be delayed.
 
 ```js
@@ -685,53 +669,54 @@ nock('http://my.server.com')
   .reply(200, '<html></html>')
 ```
 
-`delay()` could also be used as
+`delay(1000)` is an alias for `delayConnection(1000).delayBody(0)`  
+`delay({ head: 1000, body: 2000 })` is an alias for `delayConnection(1000).delayBody(2000)`  
+Both of which are covered in detail below.
 
-```
-delay({
-   head: headDelayInMs,
-   body: bodyDelayInMs
-})
-```
+#### Delay the connection
+
+You are able to specify the number of milliseconds that your connection should be idle before it starts to receive the response.
 
-for example
+To simulate a socket timeout, provide a larger value than the timeout setting on the request.
 
 ```js
 nock('http://my.server.com')
   .get('/')
-  .delay({
-    head: 2000, // header will be delayed for 2 seconds, i.e. the whole response will be delayed for 2 seconds.
-    body: 3000, // body will be delayed for another 3 seconds after header is sent out.
-  })
+  .delayConnection(2000) // 2 seconds
   .reply(200, '<html></html>')
+
+req = http.request('http://my.server.com', { timeout: 1000 })
 ```
 
-### Delay the connection
+Nock emits timeout events almost immediately by comparing the requested connection delay to the timeout parameter passed to `http.request()` or `http.ClientRequest#setTimeout()`.  
+This allows you to test timeouts without using fake timers or slowing down your tests.
+If the client chooses to _not_ take an action (e.g. abort the request), the request and response will continue on as normal, after real clock time has passed.
+
+##### Technical Details
+
+Following the `'finish'` event being emitted by `ClientRequest`, Nock will wait for the next event loop iteration before checking if the request has been aborted.
+At this point, any connection delay value is compared against any request timeout setting and a [`'timeout'`](https://nodejs.org/api/http.html#http_event_timeout) is emitted when appropriate from the socket and the request objects.
+A Node timeout timer is then registered with any connection delay value to delay real time before checking again if the request has been aborted and the [`'response'`](http://nodejs.org/api/http.html#http_event_response) is emitted by the request.
 
-`delayConnection(1000)` is equivalent to `delay({ head: 1000 })`.
+A similar method, `.socketDelay()` was removed in version 13. It was thought that having two methods so subtlety similar was confusing.  
+The discussion can be found at https://github.com/nock/nock/pull/1974.
 
-### Socket timeout
+#### Delay the response body
 
-You are able to specify the number of milliseconds that your connection should be idle, to simulate a socket timeout.
+You are able to specify the number of milliseconds that the response body should be delayed.  
+This is the time between the headers being received and the body starting to be received.
 
 ```js
 nock('http://my.server.com')
   .get('/')
-  .socketDelay(2000) // 2 seconds
+  .delayBody(2000) // 2 seconds
   .reply(200, '<html></html>')
 ```
 
-To test a request like the following:
-
-```js
-req = http.request('http://my.server.com', res => {
-  ...
-})
-req.setTimeout(1000, () => { req.abort() })
-req.end()
-```
+##### Technical Details
 
-NOTE: the timeout will be fired immediately, and will not leave the simulated connection idle for the specified period of time.
+Following the [`'response'`](http://nodejs.org/api/http.html#http_event_response) being emitted by `ClientRequest`,
+Nock will register a timeout timer with the body delay value to delay real time before the [IncomingMessage](http://nodejs.org/api/http.html#http_http_incomingmessage) emits its first `'data'` or the `'end'` event.
 
 ### Chaining
 

diff --git a/lib/intercept.js b/lib/intercept.js
@@ -281,6 +281,8 @@ function overrideClientRequest() {
       debug('using', interceptors.length, 'interceptors')
 
       //  Use filtered interceptors to intercept requests.
+      // TODO: this shouldn't be a class anymore
+      // the overrider explicitly overrides methods and attrs on the request so the `assign` below should be removed.
       const overrider = new InterceptedRequestRouter({
         req: this,
         options,

diff --git a/lib/interceptor.js b/lib/interceptor.js
@@ -73,7 +73,6 @@ module.exports = class Interceptor {
 
     this.delayBodyInMs = 0
     this.delayConnectionInMs = 0
-    this.delaySocketInMs = 0
 
     this.optional = false
 
@@ -599,7 +598,7 @@ module.exports = class Interceptor {
    * @return {Interceptor} - the current interceptor for chaining
    */
   delayBody(ms) {
-    this.delayBodyInMs += ms
+    this.delayBodyInMs = ms
     return this
   }
 
@@ -610,26 +609,7 @@ module.exports = class Interceptor {
    * @return {Interceptor} - the current interceptor for chaining
    */
   delayConnection(ms) {
-    this.delayConnectionInMs += ms
-    return this
-  }
-
-  /**
-   * @private
-   * @returns {number}
-   */
-  getTotalDelay() {
-    return this.delayBodyInMs + this.delayConnectionInMs
-  }
-
-  /**
-   * Make the socket idle for a certain number of ms (simulated).
-   *
-   * @param  {integer} ms - Number of milliseconds to wait
-   * @return {Interceptor} - the current interceptor for chaining
-   */
-  socketDelay(ms) {
-    this.delaySocketInMs = ms
+    this.delayConnectionInMs = ms
     return this
   }
 }
diff --git a/lib/playback_interceptor.js b/lib/playback_interceptor.js
@@ -137,15 +137,15 @@ function playbackInterceptor({
     interceptor.scope.emit('request', req, interceptor, requestBodyString)
 
     if (typeof interceptor.errorMessage !== 'undefined') {
-      interceptor.markConsumed()
-
       let error
       if (typeof interceptor.errorMessage === 'object') {
         error = interceptor.errorMessage
       } else {
         error = new Error(interceptor.errorMessage)
       }
-      common.setTimeout(() => emitError(error), interceptor.getTotalDelay())
+
+      const delay = interceptor.delayBodyInMs + interceptor.delayConnectionInMs
+      common.setTimeout(emitError, delay, error)
       return
     }
 
@@ -291,7 +291,7 @@ function playbackInterceptor({
       response.emit('error', err)
     })
 
-    const { delayBodyInMs, delayConnectionInMs, delaySocketInMs } = interceptor
+    const { delayBodyInMs, delayConnectionInMs } = interceptor
 
     function respond() {
       if (req.aborted) {
@@ -310,7 +310,7 @@ function playbackInterceptor({
       common.setTimeout(() => bodyAsStream.resume(), delayBodyInMs)
     }
 
-    socket.applyDelay(delaySocketInMs + delayConnectionInMs)
+    socket.applyDelay(delayConnectionInMs)
     common.setTimeout(respond, delayConnectionInMs)
   }
 

diff --git a/lib/socket.js b/lib/socket.js
@@ -20,12 +20,8 @@ module.exports = class Socket extends EventEmitter {
     this.destroyed = false
     this.connecting = true
 
-    // totalDelay that has already been applied to the current
-    // request/connection, timeout error will be generated if
-    // it is timed-out.
-    this.totalDelayMs = 0
-    // Maximum allowed delay. Null means unlimited.
-    this.timeoutMs = null
+    // Maximum allowed delay. 0 means unlimited.
+    this.timeout = 0
 
     const ipv6 = options.family === 6
     this.remoteFamily = ipv6 ? 'IPv6' : 'IPv4'
@@ -48,16 +44,22 @@ module.exports = class Socket extends EventEmitter {
   }
 
   setTimeout(timeoutMs, fn) {
-    this.timeoutMs = timeoutMs
+    this.timeout = timeoutMs
     if (fn) {
       this.once('timeout', fn)
     }
+    return this
   }
 
+  /**
+   * Artificial delay that will trip socket timeouts when appropriate.
+   *
+   * Doesn't actually wait for time to pass.
+   * Timeout events don't necessarily end the request.
+   * While many clients choose to abort the request upon a timeout, Node itself does not.
+   */
   applyDelay(delayMs) {
-    this.totalDelayMs += delayMs
-
-    if (this.timeoutMs && this.totalDelayMs > this.timeoutMs) {
+    if (this.timeout && delayMs > this.timeout) {
       debug('socket timeout')
       this.emit('timeout')
     }

diff --git a/tests/setup.js b/tests/setup.js
@@ -16,6 +16,7 @@ chai.use(sinonChai)
 
 afterEach(function () {
   nock.restore()
+  nock.abortPendingRequests()
   nock.cleanAll()
   nock.enableNetConnect()
   nock.emitter.removeAllListeners()

diff --git a/tests/test_delay.js b/tests/test_delay.js
@@ -312,4 +312,68 @@ describe('`delayConnection()`', () => {
       100
     )
   })
+
+  it('emits a timeout - with setTimeout', done => {
+    nock('http://example.test').get('/').delayConnection(10000).reply(200, 'OK')
+
+    const onEnd = sinon.spy()
+
+    const req = http.request('http://example.test', res => {
+      res.once('end', onEnd)
+    })
+
+    req.setTimeout(5000, () => {
+      expect(onEnd).not.to.have.been.called()
+      done()
+    })
+
+    req.end()
+  })
+
+  it('emits a timeout - with options.timeout', done => {
+    nock('http://example.test').get('/').delayConnection(10000).reply(200, 'OK')
+
+    const onEnd = sinon.spy()
+
+    const req = http.request('http://example.test', { timeout: 5000 }, res => {
+      res.once('end', onEnd)
+    })
+
+    req.on('timeout', function () {
+      expect(onEnd).not.to.have.been.called()
+      done()
+    })
+
+    req.end()
+  })
+
+  it('does not emit a timeout when timeout > delayConnection', done => {
+    const responseText = 'okeydoke!'
+    const scope = nock('http://example.test')
+      .get('/')
+      .delayConnection(300)
+      .reply(200, responseText)
+
+    const req = http.request('http://example.test', res => {
+      res.setEncoding('utf8')
+
+      let body = ''
+
+      res.on('data', chunk => {
+        body += chunk
+      })
+
+      res.once('end', () => {
+        expect(body).to.equal(responseText)
+        scope.done()
+        done()
+      })
+    })
+
+    req.setTimeout(60000, () => {
+      expect.fail('socket timed out unexpectedly')
+    })
+
+    req.end()
+  })
 })
diff --git a/tests/test_nock_lifecycle.js b/tests/test_nock_lifecycle.js
@@ -106,6 +106,25 @@ describe('Nock lifecycle functions', () => {
         return true
       })
     })
+
+    it('should be safe to call in the middle of a request', done => {
+      // This covers a race-condition where cleanAll() is called while a request
+      // is in mid-flight. The request itself should continue to process normally.
+      // Notably, `cleanAll` is being called before the Interceptor is marked as
+      // consumed and removed from the global map. Having this test wait until the
+      // response event means we verify it didn't throw an error when attempting
+      // to remove an Interceptor that doesn't exist in the global map `allInterceptors`.
+      nock('http://example.test').get('/').reply()
+
+      const req = http.request('http://example.test', () => {
+        done()
+      })
+      req.once('socket', () => {
+        nock.cleanAll()
+      })
+
+      req.end()
+    })
   })
 
   describe('`isDone()`', () => {

diff --git a/tests/test_socket.js b/tests/test_socket.js
@@ -0,0 +1,32 @@
+'use strict'
+
+const http = require('http')
+const nock = require('..')
+
+require('./setup')
+
+describe('`Socket#setTimeout()`', () => {
+  it('adds callback as a one-time listener for parity with a real socket', done => {
+    nock('http://example.test').get('/').delayConnection(100).reply()
+
+    const onTimeout = () => {
+      done()
+    }
+
+    http.get('http://example.test').on('socket', socket => {
+      socket.setTimeout(50, onTimeout)
+    })
+  })
+
+  it('can be called without a callback', done => {
+    nock('http://example.test').get('/').delayConnection(100).reply()
+
+    http.get('http://example.test').on('socket', socket => {
+      socket.setTimeout(50)
+
+      socket.on('timeout', () => {
+        done()
+      })
+    })
+  })
+})