'Pool.release(): Resource not member of pool' error when using setImmediate instead of setTimeout

[blah238]

After upgrading Knex to 0.8.0, I've started seeing this error Knex:Error Pool2 - Error: Pool.release(): Resource not member of pool in my unit tests.

It only seems to happen to a couple of unit tests where, to avoid waiting for delays in my Bluebird promises, I stub setTimeout by replacing it with setImmediate. If I avoid stubbing setTimeout it works fine.

This might sound like a strange request, but could you check to see if replacing setTimeout with setImmediate in your code breaks down any logic and if it's possible to guard against that scenario?

Or if you have any suggestions for other ways of not having to wait for setTimeout in unit tests, that would be great.

[tgriesser]

It only seems to happen to a couple of unit tests where, to avoid waiting for delays in my Bluebird promises, I stub setTimeout by replacing it with setImmediate. If I avoid stubbing setTimeout it works fine.

That sounds like a generally bad idea, can you show the situations that require this? If I'm reading that correctly, it sounds like you're intentionally attempting to break the contract of Promises/A+?

[blah238]

It was suggested to me by the developer of Bluebird here: http://stackoverflow.com/questions/29785784/how-to-fake-bluebirds-timers-in-tests

I agree it seems icky but I don't know of a better way.

[tgriesser]

Oh, so you're just trying to trick the timers...

In that case you'd probably be better off stubbing Promise.delay directly:

var stubDelay = Promise.delay;
Promise.delay = function() {
  return stubDelay(1)
}

Also @petkaantonov you recommended stubbing setTimeout with setImmediate? Sounds like that could potentially break a lot of stuff, no? They don't even have the same method signature :P

[blah238]

Yeah that's essentially what I was doing at first. I'll probably end up going back to what I had originally in my SO question:

// beforeEach
this.sandbox = sinon.sandbox.create();
this.sandbox.stub(Promise, 'delay', Promise.resolve);

// afterEach
this.sandbox.restore();

[myndzi]

If you can come up with a test case that applies directly to pool2, I'll be happy to try and fix it, but I don't have a good enough conception of the entire code path with knex in the mix to really tell why this would be the case. I agree with tgriesser that this seems like a terrible idea, and I'm a bit surprised petka suggested it.

A bigger problem is that what you're doing is not actually a unit test; you're testing multiple integrated parts -- an integration test. You should not be doing things like messing with fundamental parts of the JS environment and expecting anything to go right; this sort of thing may be okay in a true unit test -- where all the code being executed is small, contained, and under your control, but it's definitely a bad tack to take with integration tests.

If you can separate the logic of your code such that the timer-based code is self-contained and stub out knex's part in the mix, then you can create a true unit test and stub away to your heart's content without concern (as long as you put everything back!)

[myndzi]

There are some ordering semantics to how setImmediate, process.nextTick, and regular timers work, but I don't know them in detail.

At a guess, perhaps the timer that waits for a graceful release of the resource is expiring immediately and causing the item to be destroyed before some other code that should happen executes, and that other code is therefore trying to call remove on a destroyed resource. You should be able to investigate the exact order of operations by running your test with DEBUG=pool2

[petkaantonov]

@tgriesser Well bluebird itself is tested with overridden setTimeout, I cannot see why that would break anything

[myndzi]

The difference is that Bluebird is actually performing unit tests, so all the code utilizing setTimeout is under your direct control and awareness; the OP is performing integration tests, so messing with the global Javascript API affects things out of his control and awareness in unexpected ways. I suppose if both knex and pool2 were also tested this way, they might be expected to work, but then again they might not work in combination anyway...

Either way, it appears that changing setTimeout, which has a duration, to behave immediately in the next turn of the event loop is throwing something out of order. My suspicion is something to do with resource removal (which may happen next tick) vs removal timeout -> destroy (which should happen after)

[myndzi]

@blah238

I think I ran it down.

Try also stubbing out clearTimeout to clearImmediate. There are still some tests that fail under pool2 this way, but they probably don't relate to your usage (for example, the test that ensures resources are reaped back down to the minimum) but the problem you originally reported is caused because clearTimeout cannot clear a setImmediate object.

[myndzi]

The above, and considerations for setInterval, are not enough to make all the tests pass; there is still the matter of reaping idle resources based on a configurable timeout; this now requires stubbing Date, too, in such a way as to cause these resources to be perceived as idle.

Sinon offers "time manipulation" functionality, I recommend you simply use that, instead of trying this attractive but ultimately too-simple hack.

Closing this issue; it is not reasonable to modify pool2's code to allow for this kind of test usage.