medikoo / memoizee

Complete memoize/cache solution for JavaScript
ISC License
1.73k stars 61 forks source link

TypeError in async.js after calling clear() #13

Closed gabmontes closed 10 years ago

gabmontes commented 10 years ago

@medikoo since upgrading to 0.3.3 from 0.2.x, I am experiencing several TypeError's in async.js after clearing the cache. The errors are basically two and I was able to consistently reproduce the problem.

The following code memoizes an async function asyncfn and clears the cache between the two calls:

var memoize = require("memoizee");

function asyncfn (val, cb) {
    setTimeout(function () {
        cb(null, val);
    }, 0);
}

var asyncmem = memoize(asyncfn, {
    async: true
});

function done(err, val) {
    console.log(val);
}

asyncmem(1, done);
asyncmem.clear();
asyncmem(2, done); // TypeError is thrown after this call

It will always break with 'TypeError: Cannot read property \'length\' of undefined', ' at .../memoizee/ext/async.js:88:67 after the second asyncmem call.

Now, if asyncfn returns an error to its callback by changing cb(null, val); in line 5 by cb(new Error(), val);, it will always break with 'TypeError: Cannot call method \'forEach\' of undefined', ' at .../memoizee/ext/async.js:94:8.

Both errors are related to calling clear() in between two calls. Can you please take a look at this issue and advice how it can be solved?

Thanks!!!

medikoo commented 10 years ago

Great thanks for reporting it. I'll look into it shortly

gabmontes commented 10 years ago

@medikoo thanks in advance!

It looks like after the call to clear(), the waiting list is cleared too. Then, after the second call the list ends up with only one callback item. When the first async calls returns, it takes the callback function in waiting but the second async call finds nothing and breaks.

As you can see, this also means that the callbacks are mixed up after the clear() call. Change the code as follows:

function done1(err, val) {
    console.log("done1", val);
}

function done2(err, val) {
    console.log("done2", val);
}

asyncmem(1, done1); // waiting = done1
asyncmem.clear(); // waiting is cleared
asyncmem(2, done2); // waiting = done2

The result is:

done2 1

TypeError: Cannot read property 'length' of undefined

mening the first async call ended and the second callback was called.

medikoo commented 10 years ago

Issue was that unique cache ids were not guaranteed between clear calls, and that was not safe for async handling, where ids lived a little longer.

Fixed and published as v0.3.4

Thanks!

gabmontes commented 10 years ago

@medikoo thanks for the quick fix!!!

Unfortunately after upgrading to v0.3.4 I still see some TypeErrors in my logs, on both lines 88 and 94. Possibly calling clear() in between other two calls is not the only case that ends up in an error. Will continue analyzing and will let you know if I find something else.

Thanks again!

medikoo commented 10 years ago

@gabmontes please submit working test case. We definitely need to coin that. Even if you're not able to provide minimal case, prepare something that reproduces that. I'll look into it