TaskGroup doesnt work well with assertion libraries like assert and should

[kksharma1618]

Mainly because it suppresses errors from callback.

Code:

var TaskGroup = require('taskgroup').TaskGroup;
var Task = require('taskgroup').Task;
var assert = require('assert');

function toTest(a, next) {
    var tasks = new TaskGroup();
    tasks.addTask('upper case', function(next) {
        a = a.toUpperCase();
        next();
    });
    tasks.done(function(err) {
        next(err, a);
    }).run();
}
function toTestWithoutTaskGroup(a, next) {
    a = a.toUpperCase();
    setTimeout(function() {
        next(null, a);
    }, 10);
}

toTest("abc", function(err, r) {
    assert.equal(r, "lmn");
});

toTestWithoutTaskGroup("abc", function(err, r) {
    assert.equal(r, "efg");
});

In above code, only the second assert (toTestWithoutTaskGroup) will report the failure. First assert will still throw the error but TaskGroup suppresses that. Maybe it shouldnt suppress errors in final callback?

[kksharma1618]

If you call done callback from next tick then it works:

var TaskGroup = require('taskgroup').TaskGroup;
var Task = require('taskgroup').Task;
var assert = require('assert');

function toTest(a, next) {
    var tasks = new TaskGroup();
    tasks.addTask('upper case', function(next) {
        a = a.toUpperCase();
        next();
    });
    tasks.done(function(err) {
        next(err, a);
    }).run();
}
function toTest2(a, next) {
    var tasks = new TaskGroup();
    tasks.addTask('upper case', function(next) {
        a = a.toUpperCase();
        next();
    });
    tasks.done(function(err) {
        setTimeout(function() {
            next(err, a);
        });
    }).run();

}
function toTestWithoutTaskGroup(a, next) {
    a = a.toUpperCase();
    setTimeout(function() {
        next(null, a);
    }, 10);
}

toTest("abc", function(err, r) {
    assert.equal(r, "lmn");
});

toTest2("abc", function(err, r) {
    assert.equal(r, "pqr");
});


toTestWithoutTaskGroup("abc", function(err, r) {
    assert.equal(r, "efg");
});

In above code assertion will report failure in toTest2 and toTestWithoutTaskGroup

[kksharma1618]

If anyone else is facing this issue, here is a quick fix.

Just add this code before your tests:

var TaskGroup = require('taskgroup').TaskGroup;
var Task = require('taskgroup').Task;
if(!TaskGroup.prototype.oldOnceDone) {
    TaskGroup.prototype.oldOnceDone = TaskGroup.prototype.onceDone;
    TaskGroup.prototype.onceDone = function(next) {
        if(typeof(next) != 'function') {
            return this;
        };
        return this.oldOnceDone(function() {
            var args = Array.prototype.slice.call(arguments);
            process.nextTick(function() {
                next.apply(null, args);
            });
        });

    };
}

[balupton]

Thanks for the report. Definitely a bug.

I wonder if that quick fix will still pass the TaskGroup test suite?

[kksharma1618]

I forked and updated whenDone, onceDone: kksharma1618@dd079a7

Test failed: https://www.dropbox.com/s/z2o8v6wp7nuq59w/Screenshot%202015-02-01%2018.46.13.png?dl=0

[balupton]

Yeah, so nextTick won't work. I wonder if it is due to the domain catching the callback when it shouldn't. I wonder if we can close the domain before the done callback is called, or if we can set the boundaries of the domain better.

[kksharma1618]

Well, I think that test failed because completion state of task group wasnt set properly when assert was done. Results were same in both actual and expected values, only difference was completed tasks count.

[balupton]

Running your original code, this is the output I get:

$ node issue17.js 

Error: The task [taskgroup 0.11598217790015042 ➞  upper case] just completed, but it had already completed earlier, this is unexpected. State information is:
{ error: 
   { [AssertionError: "ABC" == "pqr"]
     name: 'AssertionError',
     actual: 'ABC',
     expected: 'pqr',
     operator: '==',
     message: '"ABC" == "pqr"',
     domain: 
      { domain: null,
        _events: [Object],
        _maxListeners: 10,
        members: [] },
     domainThrown: true },
  previousResult: [],
  currentArguments: 
   [ { [AssertionError: "ABC" == "pqr"]
       name: 'AssertionError',
       actual: 'ABC',
       expected: 'pqr',
       operator: '==',
       message: '"ABC" == "pqr"',
       domain: [Object],
       domainThrown: true } ] }
    at Task.exit (/Users/balupton/Projects/taskgroup/out/lib/taskgroup.js:291:15)
    at Domain.emit (events.js:95:17)
    at process._fatalException (node.js:255:29)

[balupton]

Is that what you got?

[kksharma1618]

Can I see code for issue17.js?

[balupton]

var TaskGroup = require('./').TaskGroup;
var Task = require('./').Task;
var assert = require('assert');

function toTest(a, next) {
    var tasks = new TaskGroup();
    tasks.addTask('upper case', function(next) {
        a = a.toUpperCase();
        next();
    });
    tasks.done(function(err) {
        next(err, a);
    }).run();
}
function toTest2(a, next) {
    var tasks = new TaskGroup();
    tasks.addTask('upper case', function(next) {
        a = a.toUpperCase();
        next();
    });
    tasks.done(function(err) {
        setTimeout(function() {
            next(err, a);
        });
    }).run();

}
function toTestWithoutTaskGroup(a, next) {
    a = a.toUpperCase();
    setTimeout(function() {
        next(null, a);
    }, 10);
}

toTest("abc", function(err, r) {
    assert.equal(r, "lmn");
});

toTest2("abc", function(err, r) {
    assert.equal(r, "pqr");
});


toTestWithoutTaskGroup("abc", function(err, r) {
    assert.equal(r, "efg");
});

Seems you've updated it, will try with your update.

[balupton]

Distilled down to:

var TaskGroup = require('./').TaskGroup
var Task = require('./').Task
var assert = require('assert')

var tasks = new TaskGroup()
tasks.addTask('test', function(complete) {
    console.log('hello world')
    complete()
});
tasks.done(function(err) {
    throw new Error('goodbye world')
}).run()

[kksharma1618]

yep. goodbye world error shouldnt be suppressed. ideal solution will be, like you said, to fix the domain. call done listeners outside of it.

[balupton]

Seems domain doesn't have anything to do with it, disabling it did nothing.

[balupton]

Weird output with:

var TaskGroup = require('./').TaskGroup
var Task = require('./').Task
var assert = require('assert')

var myTask = new Task(function(complete) {
    console.log('hello world')
    complete()
})
myTask.done(function(err) {
    throw new Error('goodbye world')
}).run()

setTimeout(function(){
    throw new Error("world still exists! this shouldn't have happend")
}, 1000)

$ node issue17.js
hello world
Error: The task [task 0.926657605683431] just completed, but it had already completed earlier, this is unexpected. State information is:
{ error: [Error: goodbye world],
  previousResult: null,
  currentArguments: [ [Error: goodbye world] ] }
    at Task.exit (/Users/balupton/Projects/taskgroup/out/lib/taskgroup.js:291:15)
    at fire (/Users/balupton/Projects/taskgroup/out/lib/taskgroup.js:394:21)
    at b (domain.js:183:18)
    at Domain.run (domain.js:123:23)
    at Task.fire (/Users/balupton/Projects/taskgroup/out/lib/taskgroup.js:398:25)
    at Object._onImmediate (/Users/balupton/Projects/taskgroup/out/lib/taskgroup.js:414:26)
    at processImmediate [as _immediateCallback] (timers.js:354:15)
Error: The task [task 0.926657605683431] just completed, but it had already completed earlier, this is unexpected. State information is:
{ error: [Error: goodbye world],
  previousResult: null,
  currentArguments: 
   [ { [Error: The task [task 0.926657605683431] just completed, but it had already completed earlier, this is unexpected. State information is:
     { error: [Error: goodbye world],
       previousResult: null,
       currentArguments: [ [Error: goodbye world] ] }] domain: [Object], domainThrown: true } ] }
    at Task.exit (/Users/balupton/Projects/taskgroup/out/lib/taskgroup.js:291:15)
    at Domain.emit (events.js:95:17)
    at process._fatalException (node.js:255:29)

Error: The task [task 0.926657605683431] just completed, but it had already completed earlier, this is unexpected. State information is:
{ error: [Error: goodbye world],
  previousResult: null,
  currentArguments: 
   [ { [Error: The task [task 0.926657605683431] just completed, but it had already completed earlier, this is unexpected. State information is:
     { error: [Error: goodbye world],
       previousResult: null,
       currentArguments: [ [Error: goodbye world] ] }] domain: [Object], domainThrown: true } ] }
    at Task.exit (/Users/balupton/Projects/taskgroup/out/lib/taskgroup.js:291:15)
    at Domain.emit (events.js:95:17)
    at process._fatalException (node.js:255:29)

[balupton]

Found the cause, working on a fix. Leave it with me :-) 👍

[kksharma1618]

great. thanks :)

[balupton]

Got a fix locally, but I'm really tired and it has a minor breaking change. Will sleep on it and release tomorrow.

[balupton]

So the issue was actually being called by the try...catch that was used, AS well as the domain - hence why disabling domains still caused the issue. This is also why the nextTick workaround worked, as it killed the scope.

I've fixed it by removing the try...catch and limiting the scope of the domain to just where it is needed as well as manually exiting it upon task method completion.

Fix will be in v4.1.0. Releasing now.

[balupton]

Released.

[balupton]

Ended up releasing v4.2.0 that has try catch still in it, but uses your next tick idea :-) As it was needed for node 0.8 and browser compat

[kksharma1618]

Thanks for the fix. Just updated and tried again. Works.

Cool library btw, I was using async before this. Just switched to taskgroup in this project. I really like how you implemented taskgroup.

[pflannery]

just got latest from master and running the tests with node 0.12.0 give this error

Error #1:
task > errors > it should not catch errors within the completion callback: issue 17
Error: Issue 17 check did not execute correctly
    at d:\dev\github\bevry\taskgroup\out\test\taskgroup-unit-test.js:216:19
    at d:\dev\github\bevry\taskgroup\node_modules\safeps\out\lib\safeps.js:247:47
    at ChildProcess.exithandler (child_process.js:751:5)
    at ChildProcess.emit (events.js:110:17)
    at maybeClose (child_process.js:1008:16)
    at Socket.<anonymous> (child_process.js:1176:11)
    at Socket.emit (events.js:107:17)
    at Pipe.close (net.js:476:12)

[balupton]

@pflannery I'll check it out, strange as I tested it with 0.11 and iojs and worked fine