Skip to content

Commit

Permalink
fix(playback): consistently check for destroyed attribute (#2152)
Browse files Browse the repository at this point in the history
Node, as of 14.1, started migrating the Client Request terminology from
`aborted` to `destroyed`. In order to supported our current Node version
support, 10.x+, we need to check both flags. Nock was already doing this
in the router, but not during playback.
  • Loading branch information
mastermatt committed Feb 27, 2021
1 parent eb7ec88 commit b9758c8
Show file tree
Hide file tree
Showing 5 changed files with 52 additions and 7 deletions.
29 changes: 29 additions & 0 deletions lib/common.js
Expand Up @@ -673,6 +673,34 @@ function removeAllTimers() {
clearTimer(clearImmediate, immediates)
}

/**
* Check if the Client Request has been cancelled.
*
* Until Node 14 is the minimum, we need to look at both flags to see if the request has been cancelled.
* The two flags have the same purpose, but the Node maintainers are migrating from `abort(ed)` to
* `destroy(ed)` terminology, to be more consistent with `stream.Writable`.
* In Node 14.x+, Calling `abort()` will set both `aborted` and `destroyed` to true, however,
* calling `destroy()` will only set `destroyed` to true.
* Falling back on checking if the socket is destroyed to cover the case of Node <14.x where
* `destroy()` is called, but `destroyed` is undefined.
*
* Node Client Request history:
* - `request.abort()`: Added in: v0.3.8, Deprecated since: v14.1.0, v13.14.0
* - `request.aborted`: Added in: v0.11.14, Became a boolean instead of a timestamp: v11.0.0, Not deprecated (yet)
* - `request.destroy()`: Added in: v0.3.0
* - `request.destroyed`: Added in: v14.1.0, v13.14.0
*
* @param {ClientRequest} req
* @returns {boolean}
*/
function isRequestDestroyed(req) {
return !!(
req.destroyed === true ||
req.aborted ||
(req.socket && req.socket.destroyed)
)
}

module.exports = {
contentEncoding,
dataEqual,
Expand All @@ -686,6 +714,7 @@ module.exports = {
isContentEncoded,
isJSONContent,
isPlainObject,
isRequestDestroyed,
isStream,
isUtf8Representable,
mapValue,
Expand Down
6 changes: 2 additions & 4 deletions lib/intercepted_request_router.js
Expand Up @@ -103,8 +103,7 @@ class InterceptedRequestRouter {
connectSocket() {
const { req, socket } = this

// 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) {
if (common.isRequestDestroyed(req)) {
return
}

Expand Down Expand Up @@ -250,8 +249,7 @@ class InterceptedRequestRouter {
return
}

// 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) {
if (!common.isRequestDestroyed(req) && !playbackStarted) {
this.startPlayback()
}
}
Expand Down
4 changes: 2 additions & 2 deletions lib/playback_interceptor.js
Expand Up @@ -293,7 +293,7 @@ function playbackInterceptor({
const { delayBodyInMs, delayConnectionInMs } = interceptor

function respond() {
if (req.aborted) {
if (common.isRequestDestroyed(req)) {
return
}

Expand All @@ -318,7 +318,7 @@ function playbackInterceptor({
// correct events are emitted first ('socket', 'finish') and any aborts in the in the queue or
// called during a 'finish' listener can be called.
common.setImmediate(() => {
if (!req.aborted) {
if (!common.isRequestDestroyed(req)) {
start()
}
})
Expand Down
16 changes: 16 additions & 0 deletions tests/test_destroy.js
Expand Up @@ -34,4 +34,20 @@ describe('`res.destroy()`', () => {
expect.fail('should not emit error')
})
})

it('should not emit an response if destroyed first', done => {
nock('http://example.test').get('/').reply()

const req = http
.get('http://example.test/', () => {
expect.fail('should not emit a response')
})
.on('error', () => {}) // listen for error so "socket hang up" doesn't bubble
.on('socket', () => {
setImmediate(() => req.destroy())
})

// give the `setImmediate` calls enough time to cycle.
setTimeout(() => done(), 10)
})
})
4 changes: 3 additions & 1 deletion tests/test_nock_off.js
Expand Up @@ -30,7 +30,9 @@ describe('NOCK_OFF env var', () => {
.get('/')
.reply(200, 'mock')

const { body } = await got(origin, { ca: httpsServer.ca })
const { body } = await got(origin, {
https: { certificateAuthority: httpsServer.ca },
})
expect(body).to.equal(responseBody)
scope.done()
})
Expand Down

0 comments on commit b9758c8

Please sign in to comment.