Allow errors in callback to acquire, make tests stricter

[tmcw]

This commit definitely breaks the API in order to allow two things: the acquire function now calls a callback with (err, result) instead of just (result), which didn't allow for many error cases. And, in order to do cleanup from this kind of situation, destroy(callback) is now on me. so that users can call it to destroy objects that can't be repaired on their side.

[coopernurse]

Thanks for the code. Would you be opposed to making these changes backwards compatible? If the callback took: (result, err) then existing code should work.

I'm not sure I understand why the destroy() function needs to be moved. Can you help me understand the scenario here?

[tmcw]

My original intent was to make this backwards-compatible, via arguments.length. I'll give that another shot. Doing (result, err) would be simpler, but would also cut against the grain of lots of node convention.

Sure: so, the deal with destroy() is that, when acquire() fails - in my case, when a map can't be loaded because of invalid data, the product can't be released back into the pool, because it's invalid. So, in the naïve case, the pool fills with invalid objects, and if you don't call release at all, the pool is exhausted of slots. Thus the solution I'm using currently consists of calling destroy() to destroy the object and decrement the slots. An alternate solution would be just destroying any object that has an error in its callback, but possibly some objects are 'repairable'? I'm not sure what to do there.

[coopernurse]

Agreed, most node modules put err first. But let's think about this a bit more.

generic-pool doesn't impose any rules on what the payload returned from acquire() should be. In your case you need an error. Other folks may not. Either way, it's symmetric. You own the create()/destroy() behavior, so you can make them do whatever you want.

What if you did this:

    var pool = poolModule.Pool({
        name : 'mypool',
        create : function(callback) {
            var resource = someFunctionThatFailsSometimes();
            if (resource) { callback([null, resource]); }
            else { callback(["Couldn't get the resource for some reason", null]); }
        },
        destroy : function(resource) {
            if (resource && resource[1]) { 
                cleanupMyResource(resource[1]);
            }
        }
    });


    pool.acquire(function(wrapper) {
        if (wrapper[0]) {
            console.log("got an error: " + wrapper[0]);
        }
        else {
            doSomethingWithResource(wrapper[1]);
        }

        pool.release(wrapper);
    });

The if test you have to do inside your acquire callback doesn't go away if we add err, and it makes release() unclear. Do I have to release if there's an error?

    pool.acquire(function(err, resource) {
        if (err) {
            console.log("got an error: " + err);
        }
        else {
            doSomethingWithResource(resource);
        }

        // ?? or only call this in the else block above?
        pool.release(resource);
    });

Let me know if I'm missing your use case. But I think we should keep the error handling semantics up to the pool user since you have complete control over the create/destroy functions.

It's an interesting case though. I'm not sure what I would expect if MySQL were down and I did an acquire. I suppose I'd get back a connect() that failed. I need to check that behavior in my node app now.. :-)

[tmcw]

That seems decent, and made me think that it'd be pretty simple to use .apply() to actually just have callbacks get whatever number of arguments you want then to. But this clashes pretty hard with the fact that acquire expects to know what is the object if a callback isn't given - on this line.

[coopernurse]

Yeah.. The array hack in my example is admittedly not very elegant, but it plays ball with the rest of the code.

Would that workaround work for your case?

[tmcw]

It would, but I wonder if this change shouldn't be essentially a 2.0.0, to indicate the API breakage, instead of trying to maintain it through any kind of workaround. I think there are quite a few examples of other pools that need an error condition, and err, result is a pretty strong common assumption elsewhere?

[yhahn]

I've updated the branch to support either single argument or double argument callbacks in both the factory.create method and in acquire() callbacks. I've also added two tests -- for legacy callbacks and to ensure factory.create does not pollute the pool when errors occur.

@coopernurse do you want to take a look at this and see if it would address the issue on hand but also allow node-pool to retain backwards-compatibility for existing API users?

[coopernurse]

Ah.. what's the workaround if you want to use bind()? I'd like to add that to the README.md

thanks!