Delete cache entries older than max age

[keithamus]

Related to #14 although it might not fix it explicitly.

This explicitly calls delete() if the max age has passed, so that if passing a caching object that is maybe different to Map then it doesn't rely on set() in order to actually remove old entries.

This also fixes a bug where if a subsequent call to fn may throw an error (even if the original call did not - perhaps because some side-effect has changed, e.g. a filesystem - causing fn to now throw) - then the .set was never called and so older entries would remain without ever being explicitly cleared up.

[SamVerschueren]

Could you also add a test to make sure it removes the old item?

[keithamus]

Could you please propose how I would do that?

[keithamus]

Managed to get a working test for this.

[SamVerschueren]

You can just check if the size of the cache is 0 after the expired time. Not necessary to override the delete method.

[keithamus]

No - because the cache is repopulated by virtue of the second call - with every call the cache.size will always be 1. The only way to test this is to spy on the side effect of deleting from cache.

[tchristian32]

Any idea when this PR will be released? Sounds like something we want to fix.

[0x80]

@SamVerschueren @keithamus Are you two in a deadlock or what's going on?

If there is really a memory leak, fixing it would be important, no? I'm about to use this in production in several parts of my infrastructure, so I got a little worried stumbling on this issue by accident...

[keithamus]

@0x80 I dont think we're in a deadlock. The PR has tests, and I've explained why those tests are what is needed and cannot be simplified.

Happy to take more feedback from @SamVerschueren or @sindresorhus about what is needed to get this merged.

If you're about to deploy this to production - you could always publish a version on your own namespace that includes this patch.

[0x80]

@keithamus I will do that then.

If it's hard or impossible to improve the tests, and it won't hurt to merge this, then I hope @SamVerschueren or someone can approve this PR.

The last thing a memoization lib needs is a memory leak... 😄

[LinusU]

I don't see how this fixes the problem though? The delete call to the map that has been introduced here will only run just before we make a set call to the map, so it won't have any effect at all.

I think that a proper fix would cover the following test case:

test('maxAge option deletes old items', async t => {
	const f = () => 'foobar';
	const cache = new Map();
	const memoized = m(f, {maxAge: 100, cache});

	t.is(memoized(1), 'foobar');
	t.is(cache.has(1), true);

	await delay(50);
	t.is(cache.has(1), true);

	await delay(50);
	t.is(cache.has(1), false);
});

[keithamus]

@LinusU if deletions were observable outside of calls to the memoized function as per your test case that would add a lot of complexity to this library which Im unsure is necessary. Adding for e.g. a setTimeout call to clear the cache means this library does a lot more (and has more side effects) than at current.

[LinusU]

if deletions were observable outside of calls to the memoized function as per your test case that would add a lot of complexity to this library which Im unsure is necessary

This would be the only way to solve #14 though?

Since the problem that that issue raises is that any cached value will stay in the cache until a new value replaces it. This is an issue if you e.g. are caching web fetches, but only want to cache them for 5 minutes. If you at some point make 5000 different requests, for some domains that you will never access again, those 5000 responses will now be stored in memory until the process is terminated.

---

Did you understand what I meant with this?

The delete call to the map that has been introduced here will only run just before we make a set call to the map, so it won't have any effect at all.

As far as I can see, the patch that you have proposed doesn't change the behaviour of the module in any way. By extension, it also doesn't address the issue raised in #14.

[keithamus]

those 5000 responses will now be stored in memory until the process is terminated

I understand this. I think in these instances it may be preferable to manually clear. I'm just concious of side effects. Introducing timers into libraries like this can cause unpredictability - like gc pauses which become hard to debug because a library (that you might not expect to) is firing off events at somewhat unpredictable times.

Did you understand what I meant with this?

I did. While calling Map with a new set operation does indeed clear this, for my own purposes I need specifically to call delete (I'm working with a subclassed version of Map that moves operations to disk, which is where I found this never actually calls delete in the way I want it to).

Perhaps I should rephrase the description as this might not actually fix #14 in the way folks may expect it to. I've now edited the description to better fit what this attempts to resolve. To quote an important part of the new description:

This also fixes a bug where if a subsequent call to fn may throw an error (even if the original call did not - perhaps because some side-effect has changed, e.g. a filesystem - causing fn to now throw) - then the .set was never called and so older entries would remain without ever being explicitly cleared up.

[SamVerschueren]

I agree with @LinusU that it does not solve #14 and it only helps when the new call fails like you mention @keithamus. Think the changes make sense for that part.

[LinusU]

This also fixes a bug where if a subsequent call to fn may throw an error

This is good 👍

[keithamus]

I've added an additional test to explicitly cover the case of a function throwing and the cache still being deleted.

[sindresorhus]

@keithamus Thank you and sorry for the slow reply. I've released it as a patch version.