Cache and pMemoizeClear bug

[nikashitsa]

Hi, great module!

I found interesting bug with pMemoizeClear and cache.
Here is example:

import ExpiryMap from 'expiry-map'; // expiry-map@2.0.0
import pMemoize, { pMemoizeClear } from 'p-memoize'; // p-memoize@7.1.1

const memoizedFn = pMemoize(async (str) => str, {
  cache: new ExpiryMap(10000),
});

await memoizedFn('something');

const [value] = await Promise.all([
  memoizedFn('something'),
  pMemoizeClear(memoizedFn),
]);

console.log(value); // undefined

It's connected with these two lines https://github.com/sindresorhus/p-memoize/blob/52fe6052ff2287f528c954c4c67fc5a61ff21360/index.ts#L104-L105 if I remove await in if condition, result will be something as expected.

I'm not sure how to fix this issue, perhaps you have better ideas than me 😉

[sindresorhus]

Probably related to https://github.com/sindresorhus/p-memoize/pull/36

[nikashitsa]

@sindresorhus This bug happens because between two await-s if (cache && await cache.has(key)) { and return (await cache.get(key))!; cache cleaning function was executed. In asynchronous app it could happen easily.
Also ExpiryMap could clean cache right after await cache.has(key) returns true.

[sindresorhus]

Yes. The only solution I can think of is to have some kind of lock to make sure that no other cache operation are executed during that time.

[sindresorhus]

I wonder if something like this would solve it:

if (cache && await cache.has(key)) {
    const [has, value] = await Promise.all([cache.has(key), cache.get(key)]);

    if (has) {
        return value;
    }
}

It would call has twice, but I don't think that's too bad.

[nikashitsa]

Yep, this is working solution. has() and get() for synchronous cache are executed in [] one by one, so there is no chance for clearing cache function to execute.

For my project calling has twice is fine. But for those who use async remote cache, calling has one more time it will make one more network request (not sure how many such users).

[fregante]

I’m not sure this is a bug. You’re starting an async operation and immediately aborting it before it resolved.

The only issue I see is that this probably leads to a race condition between cached calls and new calls.

[nikashitsa]

@fregante this bug for async app like web server with multiple parallel incoming requests. Or in case if setInterval is executing pMemoizeClear every X minutes while other part of app is using memoizedFn for random requests.

[fregante]

You want pending checks to be treated differently from completed checks. I’d classify this as a feature request rather than a bug.

If I call “clear cache” I expect the cache to be cleared. Imagine if a pending browser request reinstated the previous cookie after the user cleared the cache, finding themselves still logged in.

fetch as @foo           response sets @foo cookie
|- - - - - - - - - - — -|

          clear cache
          |- - - -|

                         fetch as @foo
                         |- - -