feat: Canceling flows #1542

aearly · 2018-06-05T21:16:31Z

This allows you to signal that you have exited early from an async flow by callback(false) --- calling back with false as the error. All further callbacks will be ignored, and the final callback will not be called. This is handy to avoid a subtle memory leak when you exit early.

aearly · 2018-06-11T02:45:55Z

This is ready!

hargasinski

This is probably the change I'm looking forward to the most in v3, really excited about it!

The behaviour seems a little unexpected though. I assumed the final callback would still be called, with partial results, similar to the way breakLoop works for the collection methods?

I think the only function missing is eachOfArrayLike in lib/eachOf. Other than that, should retry/retryable also be cancelable?

hargasinski · 2018-06-12T04:15:24Z

lib/auto.js

@@ -189,6 +190,10 @@ export default function (tasks, concurrency, callback) {

        var taskCallback = onlyOnce(function(err, result) {


Higher up in the runTask function, we should change if (hasError) return; to if (hasError || canceled) return; so other tasks, not dependent on this one, won't continue running.

hargasinski · 2018-06-12T04:28:31Z

lib/waterfall.js

@@ -75,6 +76,10 @@ export default  function(tasks, callback) {
    }

    function next(err/*, ...args*/) {
+        if (err === false || canceled) {


nit: I think this can be simplified to if (err === false) return. next is wrapped in onlyOnce so I don't believe there's a situation were it would be called twice.

hargasinski · 2018-06-12T04:48:17Z

test/auto.js

@@ -168,6 +168,31 @@ describe('auto', function () {
        setTimeout(done, 100);
    });

+    it('auto canceled', function(done){


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) });

hargasinski · 2018-06-12T04:51:17Z

test/during.js

@@ -34,6 +34,22 @@ describe('during', function() {
        );
    });

+    it('during canceling', (done) => {


Should we also add a test for canceling during in the check?

We're going to deprecate during, making all whilst/until functions have an async test function, so no need for comprehensive test coverage.

megawac

Left some minor nits but looks promising overall, I'm excited to try and find uses for this in my projects

megawac · 2018-06-12T13:53:46Z

intro.md

@@ -173,6 +173,40 @@ async.map([1, 2, 3], AsyncSquaringLibrary.square.bind(AsyncSquaringLibrary), fun
 });
 ```

+### Subtle Memory Leaks
+
+There are cases where you might want to exit early from async flow, when calling an Async method insice another async function:


Typo -insice/+inside

megawac · 2018-06-12T13:57:05Z

lib/internal/eachOfLimit.js

            else if (value === breakLoop || (done && running <= 0)) {
                done = true;
+                if (canceled) return


Move to top of iterateeCallback for simplicity? (it would also let you avoid the !canceled check in the error condition)

megawac · 2018-06-19T16:47:25Z

lib/auto.js

@@ -102,6 +102,7 @@ export default function (tasks, concurrency, callback) {

    var results = {};
    var runningTasks = 0;
+    var canceled = false;


Typo should be cancelled

Either spelling is correct. I chose one 'l' for consistency.

damn americans

aearly · 2018-07-01T23:24:13Z

Ok, I believe I've addressed all your concerns. Thanks for the review!

And yes, I believe not calling the final callback is what we want here, as a mechanism to avoid memory leaks. In what cases would you want to break iteration, and have the final callback called?

megawac

I know you've already merged this but can we try to be consistent about semi-colon usage please.

megawac · 2018-07-03T18:18:30Z

test/auto.js

+
+    it('does not start other tasks when it has been canceled', function(done) {
+        const call_order = []
+        debugger


Misc debugger statement

hargasinski · 2018-07-04T02:19:38Z

That's a good point @aearly. Thanks for making the changes, lgtm!

nathaniel-holder · 2019-12-30T05:15:58Z

Ok, I believe I've addressed all your concerns. Thanks for the review!

And yes, I believe not calling the final callback is what we want here, as a mechanism to avoid memory leaks. In what cases would you want to break iteration, and have the final callback called?

@aearly In my code, the final callback of a waterfall calls the parent callback, so it would break my existing code to use this cancel feature. For example:

function handler(params, done) {
    async.waterfall(
        [
            function (callback) {
                console.log('step 1');
                return callback();
            },
            function (callback) {
                console.log('step 2');
                // skip remaining steps
                return callback(false);
            },
            function (callback) {
                console.log('step 3');
                return callback();
            }
        ],
        function (err) {
            console.log('final callback');
            if (err) return done(err);
            return done(null, 'data');
        }
    );
}
handler(null, (err, data) => console.log('This will never be called. err: %j, data: %j', err, data));

Is this not a use case for others?

yeya · 2020-02-23T13:28:59Z

This feature breaks the basic node API, passing false to the callback should not be treated differently than null, undefined, or any other falsy item.
It's not enough to add a line in the docs, sometimes you pass functions you didn't write by yourself.
Is there an option to cancel this behavior?

ghost · 2020-06-17T12:30:00Z

@aearly Can you take a look at @nathaniel-holder and @yeya comment?

aearly and others added 8 commits June 3, 2018 17:00

cancelable foreach

6d61acf

cancelable waterfall

e56fea9

cancellable auto

365f19c

fix lint

8fedfa3

fix tests

dfd21d0

cancelable whilst/until/during/forever

062908b

fix waterfall test. It WILL get there

cccd889

docs

93591bf

aearly changed the title ~~[wip] Canceling flows~~ feat: Canceling flows Jun 11, 2018

aearly requested review from megawac and hargasinski June 11, 2018 01:56

aearly added the feature label Jun 11, 2018

aearly added this to the 3.0 milestone Jun 11, 2018

hargasinski requested changes Jun 12, 2018

View reviewed changes

megawac approved these changes Jun 12, 2018

View reviewed changes

megawac reviewed Jun 19, 2018

View reviewed changes

aearly added 5 commits July 1, 2018 15:45

auto should not start other tasks once canceled

9952cf7

simplify waterfall, add test for arrays

c063c6b

simplify eachOf

a85a3cd

cancelable retry

ca547a5

cancelable eachOf for arrays

69a27d6

revert test tweak

4382365

aearly merged commit 53f6130 into master Jul 2, 2018

aearly deleted the canceling branch July 2, 2018 00:12

megawac reviewed Jul 3, 2018

View reviewed changes

dependabot bot mentioned this pull request Mar 16, 2021

Bump async from 2.6.3 to 3.2.0 dudleycarr/nsqjs#314

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Canceling flows #1542

feat: Canceling flows #1542

aearly commented Jun 5, 2018 •

edited

aearly commented Jun 11, 2018

hargasinski left a comment •

edited

hargasinski Jun 12, 2018

hargasinski Jun 12, 2018

hargasinski Jun 12, 2018

hargasinski Jun 12, 2018 •

edited

aearly Jun 19, 2018

megawac left a comment

megawac Jun 12, 2018

megawac Jun 12, 2018

megawac Jun 19, 2018

aearly Jun 19, 2018

megawac Jun 19, 2018

aearly commented Jul 1, 2018

megawac left a comment

megawac Jul 3, 2018

hargasinski commented Jul 4, 2018

nathaniel-holder commented Dec 30, 2019

yeya commented Feb 23, 2020

ghost commented Jun 17, 2020

		@@ -189,6 +190,10 @@ export default function (tasks, concurrency, callback) {

		var taskCallback = onlyOnce(function(err, result) {

feat: Canceling flows #1542

feat: Canceling flows #1542

Conversation

aearly commented Jun 5, 2018 • edited

aearly commented Jun 11, 2018

hargasinski left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hargasinski Jun 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

megawac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aearly commented Jul 1, 2018

megawac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hargasinski commented Jul 4, 2018

nathaniel-holder commented Dec 30, 2019

yeya commented Feb 23, 2020

ghost commented Jun 17, 2020

aearly commented Jun 5, 2018 •

edited

hargasinski left a comment •

edited

hargasinski Jun 12, 2018 •

edited