confusing behavior with nested wait.for calls

[yelworc]

Nested calls of wait.for currently cause an error ("Callback for function ... called twice") and confusing behavior. Example (gist):

function g(callback) {
    setImmediate(function () {
        callback(null, 'G');
    });
}

function f(callback) {
    var res_g = wait.for(g);
    setImmediate(function () {
        callback(null, 'F' + res_g);
    });
}

wait.launchFiber(function () {
    var res_f = wait.for(f);
    console.log('alleged result of f(): ' + res_f);
});

Output:

$ node --harmony nested-waitfor.js
alleged result of f(): G

node_modules/wait.for/waitfor.js:28
                                 throw new Error("Callback for function "+fnNa
                                       ^
Error: Callback for function f called twice. Wait.for already resumed the execution.
    at resumeCallback (node_modules/wait.for/waitfor.js:28:40)
    at Object._onImmediate (nested-waitfor.js:14:3)
    at processImmediate [as _immediateCallback] (timers.js:336:15)

The result of the inner function g is returned to the caller of the outer function f, before the error is thrown. In real-life use, this makes debugging quite a bit more difficult.

I'm not sure if this is considered a bug or incorrect usage. Looking at the code (and this related issue), the behavior seems to be by design: all the properties like callbackAlreadyCalled are attached to the fiber, so can't really support nested invocations.

If it is by design, this should be mentioned in the docs IMHO, and perhaps the error behavior can be improved.

[luciotato]

Maybe the error message should be "called twice/wait.for inside async fn"

If you think about it, It does not make sense to call wait.for inside an "async" fn (a fn with a callback param). Once you enter an async function it does not make sense to wait.for anything, best practice for async functions is to return asap.

Once you start coding an async function you must continue async and use callbacks and closures.

Feel free to alter the error message and make a pull request.

[yelworc]

I would prefer it to fail earlier - i.e. something like this pretty much at the start of applyAndWait:

if (fiber.hasOwnProperty('callbackAlreadyCalled')) {
    throw new Error('nested wait.for calls are not supported');
}

Regarding your comments: I agree that generally, it is preferable to keep the boundary between sync and async land as narrow as possible (for the sake of clean design if nothing else). When you are piecing together sync and async 3rd party components though, you usually don't have the liberty to modify their APIs, so this ideal can become difficult to achieve.
I think it would be possible to support nesting in wait.for, but it would of course make the codebase a lot more complex (call stack bookkeeping, don't resume the fiber until all async calls have returned). Since this is also a fairly specific use case, I can totally understand if it is not in the scope of wait.for.

[darrudi]

+1