Skip to content

Commit b9758c8

Browse files
mastermattmastermatt
authored
fix(playback): consistently check for destroyed attribute (#2152)
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.
1 parent eb7ec88 commit b9758c8

5 files changed

Lines changed: 52 additions & 7 deletions

File tree

‎lib/common.js‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -673,6 +673,34 @@ function removeAllTimers() {
673673
clearTimer(clearImmediate, immediates)
674674
}
675675

676+
/**
677+
* Check if the Client Request has been cancelled.
678+
*
679+
* Until Node 14 is the minimum, we need to look at both flags to see if the request has been cancelled.
680+
* The two flags have the same purpose, but the Node maintainers are migrating from `abort(ed)` to
681+
* `destroy(ed)` terminology, to be more consistent with `stream.Writable`.
682+
* In Node 14.x+, Calling `abort()` will set both `aborted` and `destroyed` to true, however,
683+
* calling `destroy()` will only set `destroyed` to true.
684+
* Falling back on checking if the socket is destroyed to cover the case of Node <14.x where
685+
* `destroy()` is called, but `destroyed` is undefined.
686+
*
687+
* Node Client Request history:
688+
* - `request.abort()`: Added in: v0.3.8, Deprecated since: v14.1.0, v13.14.0
689+
* - `request.aborted`: Added in: v0.11.14, Became a boolean instead of a timestamp: v11.0.0, Not deprecated (yet)
690+
* - `request.destroy()`: Added in: v0.3.0
691+
* - `request.destroyed`: Added in: v14.1.0, v13.14.0
692+
*
693+
* @param {ClientRequest} req
694+
* @returns {boolean}
695+
*/
696+
function isRequestDestroyed(req) {
697+
return !!(
698+
req.destroyed === true ||
699+
req.aborted ||
700+
(req.socket && req.socket.destroyed)
701+
)
702+
}
703+
676704
module.exports = {
677705
contentEncoding,
678706
dataEqual,
@@ -686,6 +714,7 @@ module.exports = {
686714
isContentEncoded,
687715
isJSONContent,
688716
isPlainObject,
717+
isRequestDestroyed,
689718
isStream,
690719
isUtf8Representable,
691720
mapValue,

‎lib/intercepted_request_router.js‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -103,8 +103,7 @@ class InterceptedRequestRouter {
103103
connectSocket() {
104104
const { req, socket } = this
105105

106-
// Until Node 14 is the minimum, we need to look at both flags to see if the request has been cancelled.
107-
if (req.destroyed || req.aborted) {
106+
if (common.isRequestDestroyed(req)) {
108107
return
109108
}
110109

@@ -250,8 +249,7 @@ class InterceptedRequestRouter {
250249
return
251250
}
252251

253-
// Until Node 14 is the minimum, we need to look at both flags to see if the request has been cancelled.
254-
if (!req.destroyed && !req.aborted && !playbackStarted) {
252+
if (!common.isRequestDestroyed(req) && !playbackStarted) {
255253
this.startPlayback()
256254
}
257255
}

‎lib/playback_interceptor.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -293,7 +293,7 @@ function playbackInterceptor({
293293
const { delayBodyInMs, delayConnectionInMs } = interceptor
294294

295295
function respond() {
296-
if (req.aborted) {
296+
if (common.isRequestDestroyed(req)) {
297297
return
298298
}
299299

@@ -318,7 +318,7 @@ function playbackInterceptor({
318318
// correct events are emitted first ('socket', 'finish') and any aborts in the in the queue or
319319
// called during a 'finish' listener can be called.
320320
common.setImmediate(() => {
321-
if (!req.aborted) {
321+
if (!common.isRequestDestroyed(req)) {
322322
start()
323323
}
324324
})

‎tests/test_destroy.js‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,4 +34,20 @@ describe('`res.destroy()`', () => {
3434
expect.fail('should not emit error')
3535
})
3636
})
37+
38+
it('should not emit an response if destroyed first', done => {
39+
nock('http://example.test').get('/').reply()
40+
41+
const req = http
42+
.get('http://example.test/', () => {
43+
expect.fail('should not emit a response')
44+
})
45+
.on('error', () => {}) // listen for error so "socket hang up" doesn't bubble
46+
.on('socket', () => {
47+
setImmediate(() => req.destroy())
48+
})
49+
50+
// give the `setImmediate` calls enough time to cycle.
51+
setTimeout(() => done(), 10)
52+
})
3753
})

‎tests/test_nock_off.js‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,9 @@ describe('NOCK_OFF env var', () => {
3030
.get('/')
3131
.reply(200, 'mock')
3232

33-
const { body } = await got(origin, { ca: httpsServer.ca })
33+
const { body } = await got(origin, {
34+
https: { certificateAuthority: httpsServer.ca },
35+
})
3436
expect(body).to.equal(responseBody)
3537
scope.done()
3638
})

0 commit comments

Comments
 (0)