BUG: Nested fibers, callback invocation in try block

[Strix-CZ]

Hello, thanks for this great library. I have found a interesting issue with it. I totally understand that it might not be a good idea to place callback invocations inside a try block, because then you might catch exceptions you never intended to. But anyway, we are not using try catch blocks in pure node.js. But in wait.for we are, so we have to be careful about it.

Here is short example in pure node.js style:

function inner(callback) {
    try {
        callback(null);
    }
    catch(error) {
        console.log('inner catch', error);
        callback(error);
    }
}

inner(function(err) {
    console.log('handler '+err);
    throw new Error('I wish I am not caught.');
});

handler null
inner catch [Error: I wish I am not caught.]
handler Error: I wish I am not caught.
--- uncaught exception ... ---

The exception is caught in the try block and this invokes the callback again. When the exception is thrown for the second time the callback invocation is no longer in the try block and we end up with uncaught exception as expected.

So how does this relates to wait.for? Let's try...

var wait = require('wait.for');

function inner(callback) {
    wait.launchFiber(function() {
        console.log('Inner fiber starting...');
        try {
            wait.for(setImmediate);
            callback(null);
        }
        catch (e) {
            console.log('Inner - catch', e);
            callback(e);
            return;
        }
        //callback (null); // Works fine - Error is no longer in the inner try block
    });
}

wait.launchFiber(function() {
    console.log('Outer fiber starting...');
    wait.for(inner);
    throw new Error('I wish I am not caught.');
});

Outer fiber starting...
Inner fiber starting...
Inner - catch [Error: I wish I am not caught.]
Outer fiber starting...
Inner fiber starting...
Inner - catch [Error: I wish I am not caught.]
Outer fiber starting...
Inner fiber starting...
Inner - catch [Error: I wish I am not caught.]
.... repeats for ever

Again, the exception is caught, which is not surprising. The problem starts when the callback is invoked for the second time - the whole outer fiber is restarted! And then the exception is thrown again....

I have spent several hours debugging my code and this is the minimal example where it works. If you put away the inner fiber or wait.for(setImmediate); it works as it should - no strange restarts.

I am just guessing - wait.for can't find the correct resume callback any more so it goes back to the beginning of the whole fiber. It should throw an error instead.

[luciotato]

wait.for is 70 LOC (and heavily commented), It's just a wrapper over node-fibers (or ES6-generators). All the hard work is done in node-fibers

Question 1) do you really need an "inner" fiber?

Wait.for proper usage is to launch a fiber to attend a request. Ideally here:

var server = http.createServer(
  function(req, res){
    console.log('req!');
    wait.launchFiber(handler,req,res); //handle in a fiber
    // keep node spinning
  }).listen(8000);

then,at function handler(req,res) and every function you call from there, you'll be able to use wait.for(ayncFn...

Normally, you don't need to launch "inner" fibers.

why do you launch a fiber as the first thing in function inner?

you can use wait.for inside function inner without launching another fiber.

Can you give me more info about your project, so I can understand why you're launching inner fibers? is the code somewhere?

[Strix-CZ]

Well, technically I don't need nested fibers, it was just coincidence. But other people might stumble upon this, that's why I am posting it :-)

I am working on medium size project which is split into several modules and every module is offering traditional callback interface to the outer world. It happened that both lower layer module and the module using it were launching fibers.

---

I have found the reason for this behaviour. Node-fibers documentation says:

run() will start execution of this Fiber, or if it is currently yielding, it will resume execution.

So when resumeCallback is called again it restarts the whole fiber. I think it is enough to modify wait.for code in this way:

//create a closure to resume on callback
var resumeCallback=function(err,data){
    if (fiber.callbackAlreadyCalled)
        throw new Error("Callback for function "+fnName+" called twice. Wait.for already resumed the execution.");

[luciotato]

ok great! I've added your if statement. Thxs