Skip to content
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

dowhilst does not test on error in cb #1287

Closed
MaerF0x0 opened this issue Sep 20, 2016 · 6 comments
Closed

dowhilst does not test on error in cb #1287

MaerF0x0 opened this issue Sep 20, 2016 · 6 comments

Comments

@MaerF0x0
Copy link

MaerF0x0 commented Sep 20, 2016

Version: 2.0.1
Env: Node v6.2.1

Reproduce code:

var async = require("async")

var count = 0;
async.doWhilst(
    function(cb_dw) {
        console.log("In Iteratee");
        return setImmediate(function(){
            switch (count) {
                case 0 :
                    return cb_dw(null, "Yes0");
                case 1 :
                    return cb_dw(new Error("Both"), "Yes1");
                case 2 :
                    return cb_dw(new Error("Just An Error"));
                default:
                    return cb_dw(new Error("Didnt expect this to be called"));
            }
        });
    },
    function(results, err) {
        console.log("Tester", err, results, count++)
        return true
    },
    function(err, results) {
        console.log("FINALLY", err, results)
    }
);

Output, with comments:

In Iteratee # Expected first run
Tester undefined Yes0 0 #Expected runs tester
In Iteratee # Expected second run because tester returned true
# === > HERE, WHY U NO Test again?
FINALLY Error: Both
    at Timeout._onTimeout (/Users/mike.graf/tmp/sep20/dowhilst.js:12:34)
    at tryOnTimeout (timers.js:224:11)
    at Timer.listOnTimeout (timers.js:198:5) undefined 

Expected: if the cb_dw receives an error it should continue to call the test function until test function returns false.

Actual: doWhilst calls the final callback as soon as cb_dw receives an error.

Longer explanation:
I am trying to do a retry that tests the error and will retry depending on the error received. So for example I may call out to a service and will retry if the error is a network style error (socket connection timeout for example) , but do not want to retry if its an authentication err (eg microservice returns InvalidCredentials error if failed auth) . If I use retry It will retry w/ bad credentials N times. So I'my trying to use doWhilst to ensure I run it atleast once, and give up if Credential Errors, but retry if network errors.

@MaerF0x0
Copy link
Author

MaerF0x0 commented Sep 20, 2016

I just read the docs closer and saw
Invoked with the non-error callback results of iteratee. http://caolan.github.io/async/docs.html#.doWhilst . So this seems its by design, though not intuitive (IMO) .

Any tip on how to achieve what i'm trying to do (test the error and maybe retry)?

@MaerF0x0
Copy link
Author

I had the idea to capture the outcome from the microservice call and just return the error as a "result" and test if (result instanceof Error) { ... stuff ... }

@MaerF0x0
Copy link
Author

MaerF0x0 commented Sep 21, 2016

This is a pattern to hack around the behavior, but it breaks the typical async pattern:

var myservice = require('myservice'); 

async.doWhilst(
    function(cb_dw) {
         // Call the service, "catch" the error and pass it to the test function
         return myservice.something("do it", function(err, result) {
             if (err) { 
                 return cb_dw(null, err);
             } else {
                 return cb_dw(null, result);
             });
    }, function(result) {
          // Test function determines if result is actually a retriable error, 
          //  say a network communication issue
          if (result instanceof NetworkError) {
              return true;
          }
          return false;
    }, function(err, result) {
        if (result instanceof Error) {
          err = result;
          result = undefined;
        }
    }
);

@ghost
Copy link

ghost commented Nov 16, 2016

I just ran into the same(?) issue in async.whilst(). the documentation states:

callback function
A callback which is called after the test function has failed and repeated execution of iteratee has stopped. callback will be passed an error and any arguments passed to the final iteratee's callback. Invoked with (err, [results]);

from that documentation I did not realize that calling the iteratee's callback() with an error would immediately jump here.

I also would like the whilst() function to behave as documented, meaning that I am allowed to call the iteratee's callback() with an error, and that still makes another call into test() and, while true, into iteratee() again.

@aearly
Copy link
Collaborator

aearly commented Apr 7, 2017

I think we should keep it as is (and perhaps make the docs more clear).

whilst and its relatives are trying to be async versions of while-loops. If I analogize your example to may it sync with do while, you get:

var count = 0;
var result, err;

try {
    do {
        result = (function (){
            switch (count) {
                case 0 :
                    return "Yes0";
                case 1 :
                    throw new Error("Both");
                case 2 :
                    throw new Error("Just An Error");
                default:
                    throw new Error("Didnt expect this to be called");
            }
        })();
    } while(test(result));
} catch (e) {
    err = e;
}

console.log("FINALLY", err, result);

function test(results) {
    console.log("Tester", err, results, count++);
    return true;
}

You wouldn't expect test() to be called a second time after you throw in the second iteration of the loop. Execution would jump to the catch block, exiting the loop. I think what we are doing with doWhilst is "correct" in regards to the synchronous behavior we're trying to emulate.

For the kind of control flow you want, there are several options. We have retry, for repeatedly calling an async function until it doesn't error (with error-filtering support), reflect for wrapping an async function and making it always succeed, passing the error or result in an object, and soon, tryEach (#1365) for running several async functions where only one is expected to succeed.

@aearly aearly closed this as completed Apr 8, 2017
@Alex-Ayala-Bonilla
Copy link

It depends of the module version you are using it

version 2.6 is a function , 3.1 is asyncFunc

From 2.6

Name Type Description
iteratee AsyncFunction A function which is called each time test passes. Invoked with (callback).
test function synchronous truth test to perform after each execution of iteratee. Invoked with any non-error callback results of iteratee.
callback function  A callback which is called after the test function has failed and repeated execution of iteratee has stopped. callback will be passed an error and any arguments passed to the final iteratee's callback. Invoked with (err, [results]);

*From 3.1

Name Type Description
iteratee AsyncFunction A function which is called each time test passes. Invoked with (callback).
test AsyncFunction asynchronous truth test to perform after each execution of iteratee. Invoked with (...args, callback), where ...args are the non-error args from the previous callback of iteratee.
callback function  A callback which is called after the test function has failed and repeated execution of iteratee has stopped. callback will be passed an error and any arguments passed to the final iteratee's callback. Invoked with (err, [results]);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants