-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
v6 is broken due to promiseCache #31
Comments
// @Richienb |
If I understand correctly, after removing values from Perhaps we could instead check whether
This would be breaking since promise properties will no longer be preserved between calls. |
|
@Richienb Sorry, that is what I meant. Checking whether something exists in 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"... |
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 |
But when would we run this check? |
@Richienb https://github.com/sindresorhus/p-memoize/pull/32/files#diff-dcdc3e0b3362edb8fec2a51d3fa51f8fb8af8f70247e06d9887fa934834c9122R113 ? This would effectively “gc” the |
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? |
Wdym "iterating" the map? Wouldn't it be a simple "if |
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. |
Ah, true enough. |
We should still try to solve the memory leak problem, though. |
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 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
The text was updated successfully, but these errors were encountered: