v6 is broken due to promiseCache

[JaneJeon]

Hi! I maintain a project that relies on p-memoize and I noticed that it started breaking when I upgraded to p-memoize v6. I tracked down the core of the problem to https://github.com/sindresorhus/p-memoize/compare/v5.0.1...v6.0.0#diff-dcdc3e0b3362edb8fec2a51d3fa51f8fb8af8f70247e06d9887fa934834c9122R107

In particular, the fact that the module relies on promiseCache (which is literally just a Map that is not configurable) to provide the actual return value means that L107-109 makes the caching behaviour rely on the promiseCache (again, which is just a Map), not the actual cache.

This screws with TTLs and mocking and all sorts of edge case behaviour.

As far as I can tell, removing L107-109 and only relying on the actual cache to check the existence of a cache key would be enough to resolve this bug and get the cache behaviour to actually rely on what we provide, but I'm also worried about what relying on a Map that is invisible to the user might mean in terms of, say, memory leaks and whatnot.

Again, the core of the issue here is that parts of the module are not completely successful in making promiseCache mimic the behaviour of the user-provided cache, and that causes bugs and memory leaks, so I'd recommend 1. Fixing L107-109 first and foremost and getting v6.0.1 out to the users and 2. Adding additional tests to the module to ensure that promiseCache behaviour does not step out of line from cache's. This might involve refactoring on your end, but as it is, v6 is broken.

Thanks, Jane

[sindresorhus]

// @Richienb

[Richienb]

If I understand correctly, after removing values from cache, the value in promiseCache still persists. For this sort of edge case, we could simply expose promiseCache as an option alongside cache.

Perhaps we could instead check whether cache contains a value before checking promiseCache. Otherwise, we could consider changing which cache takes priority and how values are cached. However, both of these methods are slightly breaking.

removing L107-109 and only relying on the actual cache to check the existence of a cache key

This would be breaking since promise properties will no longer be preserved between calls.

[Richienb]

p-memoize juggles promises and promise values in such a specific way, this might just be intended behaviour. See https://github.com/sindresorhus/p-memoize/issues/3#issuecomment-926326208 for potential foreshadowing.

[JaneJeon]

This would be breaking since promise properties will no longer be preserved between calls.

Perhaps we could instead check whether cache contains a value before checking promiseCache.

@Richienb Sorry, that is what I meant. Checking whether something exists in cache rather than relying on promiseCache to tell us if something is in the cache.

Also I'd hate to see bugs like "not respecting TTLs", "value existing in one cache but not the other" and "literal memory leak" be considered an "intended behaviour"...

[JaneJeon]

Thanks for fixing the first issue. @Richienb correct me if I'm wrong, but this still doesn't solve the memory leak issue, right? I feel like deleting entries from promiseCache when the entry in cache is not there would be a good problem to solve it?

[Richienb]

deleting entries from promiseCache when the entry in cache is not there

But when would we run this check?

[JaneJeon]

@Richienb https://github.com/sindresorhus/p-memoize/pull/32/files#diff-dcdc3e0b3362edb8fec2a51d3fa51f8fb8af8f70247e06d9887fa934834c9122R113 ?

This would effectively “gc” the promiseCache and resolve the problem, no? Obviously you’d need to test the everloving crap out of this to ENSURE there is no memory leak, but seems like a solution to me

[Richienb]

I'm not sure iterating over the map every time the function is called is a good idea. Does the performance hit outweigh the memory gain?

[JaneJeon]

Wdym "iterating" the map? Wouldn't it be a simple "if cache doesn't have a key but promisecache does, delete it", in which case it would be O(1)? You're looking up a map by a specific key, you're not "iterating over" anything! @Richienb

[Richienb]

If you're only checking the specific key that is passed to the cache, then you are only garbage-collecting the keys that are being used. The keys that stop being used will never be checked.

[JaneJeon]

Ah, true enough.

[JaneJeon]

We should still try to solve the memory leak problem, though.