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

Result of async.timeout() only works the first time you call it. #1418

Closed
acjohnso25 opened this issue May 21, 2017 · 5 comments
Closed

Result of async.timeout() only works the first time you call it. #1418

acjohnso25 opened this issue May 21, 2017 · 5 comments
Labels

Comments

@acjohnso25
Copy link

acjohnso25 commented May 21, 2017

What version of async are you using? 2.4.0

Which environment did the issue occur in (Node version/browser version) Node.js 4.7.0

What did you do? Please include a minimal reproducable case illustrating issue.

const async = require('async');

var asyncFn = function (i, done) {
    if (i < 1) {
        setTimeout(done, 200);
    } else {
        setImmediate(done);
    }
};

// wrap function once, call 10 times
var timeoutFn = async.timeout(asyncFn, 100);
var j = 0;
async.whilst(
    function () {
        return j < 10;
    },
    function (done) {
        timeoutFn(j++, function (err) {
            console.log('Test 1 #' + j, err);
            done();
        });
    },
    function (err) {
    }
);

What did you expect to happen? Should only get timeout error the first time it's called.

What was the actual result? Timeout occurs every time it's called, even though the underlying function only takes too long the first time.

@acjohnso25
Copy link
Author

acjohnso25 commented May 21, 2017

This may actually be by design, but if so it wasn't what I expected, and it wasn't indicated in the documentation from what I could see.

Simple workaround is to instead simply wrap the function each time before you call it, i.e.:

// wrap function 10 times
var k = 0;
async.whilst(
    function () {
        return k < 10;
    },
    function (done) {
        async.timeout(asyncFn, 100)(k++, function (err) {
            console.log('Test 2 #' + k, err);
            done();
        });
    },
    function (err) {
    }
);

@hargasinski
Copy link
Collaborator

Hi @acjohnso25, I believe you're right that this is bug. It should only give a ETIMEDOUT error on the first call. Thanks for the report, we'll fix as soon as possible.

@acjohnso25
Copy link
Author

I guess it's more correct to say that it works until the first timeout...then it always times out. For example, if you change asyncFn to this:

var asyncFn = function (i, done) {
    if (i === 5) {
        setTimeout(done, 200);
    } else {
        setImmediate(done);
    }
};

...then what you see is:

Test 1 #1 undefined
Test 1 #2 undefined
Test 1 #3 undefined
Test 1 #4 undefined
Test 1 #5 undefined
Test 1 #6 { [Error: Callback function "anonymous" timed out.] code: 'ETIMEDOUT' }
Test 1 #7 { [Error: Callback function "anonymous" timed out.] code: 'ETIMEDOUT' }
Test 1 #8 { [Error: Callback function "anonymous" timed out.] code: 'ETIMEDOUT' }
Test 1 #9 { [Error: Callback function "anonymous" timed out.] code: 'ETIMEDOUT' }
Test 1 #10 { [Error: Callback function "anonymous" timed out.] code: 'ETIMEDOUT' }

@aearly aearly closed this as completed in 4f6ece1 May 22, 2017
aearly added a commit that referenced this issue May 22, 2017
Allow functions wrapped in `timeout` to be called multiple times (fixes #1418)
@aearly
Copy link
Collaborator

aearly commented May 22, 2017

Yeesh, I almost feel like we should do patch releases back to 2.0.0.

@aearly
Copy link
Collaborator

aearly commented May 22, 2017

Fixed in v2.4.1

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

No branches or pull requests

3 participants