nock.cleanAll() doesn't stop delayed requests

[ikokostya]

Environment info

• Node.js version: 8.11.1
• nock version: 9.2.5

Code example

The following code example demonstrates that nock.cleanAll() call doesn't remove delayed requests from Node.js event loop:

'use strict';

const nock = require('nock');
const got = require('got');

nock('http://test')
    .get('/')
    .query(true)
    .delay(5000)
    .reply(200);

const start = Date.now();

(async () => {
    try {
        await got('http://test', {timeout: 100});
    } catch (err) {
        console.error('got error "%s" after %s ms', err.message, Date.now() - start);
        nock.cleanAll();
    }
})();

process.on('exit', () => {
    console.log('exit after %s ms', Date.now() - start);
})

Actual behavior

Node.js process waits while 5 seconds timeout will be reached:

$ node test.js
got error "Request timed out" after 108 ms
exit after 5018 ms

Expected behavior

nock.cleanAll() method removes all request timeouts.

More information

This causes problem with mocha >= 4.0.0, because Mocha will no longer force the process to exit once all tests complete. See https://github.com/mochajs/mocha/blob/master/CHANGELOG.md#default-behavior.

Related issues

• delayConnection causes hang when used with request.abort() #867

[gr2m]

I agree, it’d be great if it would clean out all timeouts. I’d happy to review a PR if you or anyone else would like to give it a go. A PR with a failing test would already be a great start, we can take it from there

[TLPcoder]

There are a few solutions to this one is a simple hack for this which would clear all setTimeouts in the queue but that obviously would include ones not created by nock, not ideal. The other solution would require a singleton which stores all timeout ids which nock creates.

[paulmelnikow]

Thanks @TLPcoder for opening a PR to address this! 🙌

I'm re-tagging this as a feature, as I'm not sure this functionality belongs in clearAll(). Here's what I posted on the issue:

[...] I'm not sure I think it belongs in the call to clearAll(). That function's job is to remove interceptors from the list. In contrast, what's happening here is that we're cancelling playback of an interceptor that has already matched and is in process.

I'd suggest exporting this as a new function, maybe calling it nock.abortRequests() or nock.abortPendingRequests(). The test name in common seems fine, "timeouts" being the implementation.

In RFC-001 I advocated creating a new lifecycle method to reset nock to its initial state.

I am not sure whether aborting current requests should be added to that reset. There is a bit of discussion in #1118 about Mocha leaving the process running until the pending request flushes, though I think I could use a bit more clarification about the use case and why aborting the requests is what's desirable. (It's not the behavior of real HTTP to abort pending requests when tests finish.) The reason Mocha doesn't quit the process anymore is to not paper over sloppy cleanup. I'm happy to let people force automatic cleanup if they want it, though others may prefer to clean things up on their own. Mocha's choice isn't everyone's cup of tea but it's arguably a good default.

That reset function doesn't exist yet; as of now you need to run nock.restore(); nock.cleanAll(); nock.enableNetConnect(); nock.activate() and when we add nock.reset() I'd suggest we stick with those four calls to start. And that developers call nock.abortPendingRequests() followed by nock.reset() if they're experiencing this problem. We can gather feedback on the use cases over time before deciding to include that reset function by default.

It's looking like this will be resolved by adding a new lifecycle method called e.g. nock.abortPendingRequests().

[nockbot]

🎉 This issue has been resolved in version 11.6.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀