medikoo / memoizee

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

Rejecting the Promise does not invalidate the cache #84

Closed SzybkiSasza closed 7 years ago

SzybkiSasza commented 7 years ago

Hi! I want to begin with the claim that I really appreciate this library! It's feature rich and helped us a lot with solving some of the issues regarding caching the requests.

However, I was trying to memoize function returning a Promise and run into a problem of the cache not being invalidated in the case the Promise is rejected

As an example, I prepared following code, based on the README example:

var memoize = require("memoizee");

var afn = function (a, b) {
    console.log('CALLED');
    return new Promise(function (res, rej) { 
        rej(new Error('Nonono')); 
    });
};

memoized = memoize(afn, { promise: 'then' });

memoized().catch(function() {
    return memoized();
})

The output will show CALLED once and then it will throw, instead of showing CALLED two times and throwing. Seems like a problem is happening regardless of Promise implementation used.

medikoo commented 7 years ago

@SzybkiSasza Thanks for report, I looked into it closer, and in case of your snippet, where you call memoized immediately in promise.catch() callback, it's actually expected, let me explain:

Promise native's then comes with side effect of error-swallowing. Code executed in callback is run as within try/catch, where eventual errors are swallowed. Some engines do provide mechanism to inform about swallowed errors, but that's not guaranteed by any spec, and more importantly, even if reported, they're not reported as exceptions, so easily can come as unnoticed

memoizee to avoid jumping into that hole of uncertainness, processes all promise results in next tick (so side-effects free context), see ->https://github.com/medikoo/memoizee/blob/b85da90af7e493e904a8baefeefc6343ce372dd7/ext/promise.js#L73-L78

Here your second memoized invocation is run before that next tick hence effect you have. You can see two CALLED if you do:

memoized().catch(function () {
  process.nextTick(function () { memoized(); });
});

So to avoid that, you need to either re-process query earliest a tick later, or rely on promise implementation that implements finally, or best if implements both done and finally but with smart handling of onFail callback in done.

I've updated the library to support four different modes (be sure to use ^0.4.8): then, then:finally, done, done:finally, and commented on implications that come with each of it. See -> https://github.com/medikoo/memoizee#important-notice-on-internal-promises-handling