-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Canceling flows #1542
feat: Canceling flows #1542
Changes from all commits
6d61acf
e56fea9
365f19c
8fedfa3
dfd21d0
062908b
cccd889
93591bf
9952cf7
c063c6b
a85a3cd
ca547a5
69a27d6
4382365
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -102,6 +102,7 @@ export default function (tasks, concurrency, callback) { | |
|
||
var results = {}; | ||
var runningTasks = 0; | ||
var canceled = false; | ||
var hasError = false; | ||
|
||
var listeners = Object.create(null); | ||
|
@@ -156,6 +157,7 @@ export default function (tasks, concurrency, callback) { | |
} | ||
|
||
function processQueue() { | ||
if (canceled) return | ||
if (readyTasks.length === 0 && runningTasks === 0) { | ||
return callback(null, results); | ||
} | ||
|
@@ -189,6 +191,10 @@ export default function (tasks, concurrency, callback) { | |
|
||
var taskCallback = onlyOnce(function(err, result) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Higher up in the |
||
runningTasks--; | ||
if (err === false) { | ||
canceled = true | ||
return | ||
} | ||
if (arguments.length > 2) { | ||
result = slice(arguments, 1); | ||
} | ||
|
@@ -200,7 +206,7 @@ export default function (tasks, concurrency, callback) { | |
safeResults[key] = result; | ||
hasError = true; | ||
listeners = Object.create(null); | ||
|
||
if (canceled) return | ||
callback(err, safeResults); | ||
} else { | ||
results[key] = result; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -168,6 +168,63 @@ describe('auto', function () { | |
setTimeout(done, 100); | ||
}); | ||
|
||
it('auto canceled', function(done){ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should also add a test for ensuring other functions don't get started when one is canceled. Something along the lines of it('does not start other tasks when it has been canceled', function(done) {
const call_order = []
async.auto({
task1: function(callback) {
call_order.push(1);
// defer calling task2, so task3 has time to stop execution
async.setImmediate(callback);
},
task2: ['task1', function(/*results, callback*/) {
call_order.push(2);
throw new Error('task2 should not be called');
}],
task3: function(callback) {
call_order.push(3);
callback(false);
},
task4: ['task3', function(/*results, callback*/) {
call_order.push(4);
throw new Error('task4 should not be called');
}]
},
function() {
throw new Error('should not get here')
});
setTimeout(() => {
expect(call_order).to.eql([1,3])
done()
}, 25)
}); |
||
const call_order = [] | ||
async.auto({ | ||
task1: function(callback){ | ||
call_order.push(1) | ||
callback(false); | ||
}, | ||
task2: ['task1', function(/*results, callback*/){ | ||
call_order.push(2) | ||
throw new Error('task2 should not be called'); | ||
}], | ||
task3: function(callback){ | ||
call_order.push(3) | ||
callback('testerror2'); | ||
} | ||
}, | ||
function(){ | ||
throw new Error('should not get here') | ||
}); | ||
setTimeout(() => { | ||
expect(call_order).to.eql([1, 3]) | ||
done() | ||
}, 10); | ||
}); | ||
|
||
it('does not start other tasks when it has been canceled', function(done) { | ||
const call_order = [] | ||
debugger | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Misc debugger statement |
||
async.auto({ | ||
task1: function(callback) { | ||
call_order.push(1); | ||
// defer calling task2, so task3 has time to stop execution | ||
async.setImmediate(callback); | ||
}, | ||
task2: ['task1', function( /*results, callback*/ ) { | ||
call_order.push(2); | ||
throw new Error('task2 should not be called'); | ||
}], | ||
task3: function(callback) { | ||
call_order.push(3); | ||
callback(false); | ||
}, | ||
task4: ['task3', function( /*results, callback*/ ) { | ||
call_order.push(4); | ||
throw new Error('task4 should not be called'); | ||
}] | ||
}, | ||
function() { | ||
throw new Error('should not get here') | ||
}); | ||
|
||
setTimeout(() => { | ||
expect(call_order).to.eql([1, 3]) | ||
done() | ||
}, 25) | ||
}); | ||
|
||
it('auto no callback', function(done){ | ||
async.auto({ | ||
task1: function(callback){callback();}, | ||
|
@@ -185,7 +242,7 @@ describe('auto', function () { | |
it('auto error should pass partial results', function(done) { | ||
async.auto({ | ||
task1: function(callback){ | ||
callback(false, 'result1'); | ||
callback(null, 'result1'); | ||
}, | ||
task2: ['task1', function(results, callback){ | ||
callback('testerror', 'result2'); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,22 @@ describe('during', function() { | |
); | ||
}); | ||
|
||
it('during canceling', (done) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we also add a test for canceling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're going to deprecate during, making all whilst/until functions have an async test function, so no need for comprehensive test coverage. |
||
let counter = 0; | ||
async.during( | ||
cb => cb(null, true), | ||
cb => { | ||
counter++ | ||
cb(counter === 2 ? false : null); | ||
}, | ||
() => { throw new Error('should not get here')} | ||
); | ||
setTimeout(() => { | ||
expect(counter).to.equal(2); | ||
done(); | ||
}, 10) | ||
}) | ||
|
||
it('doDuring', function(done) { | ||
var call_order = []; | ||
|
||
|
@@ -95,4 +111,36 @@ describe('during', function() { | |
} | ||
); | ||
}); | ||
|
||
it('doDuring canceling', (done) => { | ||
let counter = 0; | ||
async.doDuring( | ||
cb => { | ||
counter++ | ||
cb(counter === 2 ? false : null); | ||
}, | ||
cb => cb(null, true), | ||
() => { throw new Error('should not get here')} | ||
); | ||
setTimeout(() => { | ||
expect(counter).to.equal(2); | ||
done(); | ||
}, 10) | ||
}) | ||
|
||
it('doDuring canceling in test', (done) => { | ||
let counter = 0; | ||
async.doDuring( | ||
cb => { | ||
counter++ | ||
cb(null, counter); | ||
}, | ||
(n, cb) => cb(n === 2 ? false : null, true), | ||
() => { throw new Error('should not get here')} | ||
); | ||
setTimeout(() => { | ||
expect(counter).to.equal(2); | ||
done(); | ||
}, 10) | ||
}) | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo should be
cancelled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either spelling is correct. I chose one 'l' for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
damn americans