Cache not shrinking with timeouts

[kavika-1]

Awesome just right library, thanks!

Running into some occasional out of memory issues. Timeout set to 5 minutes. So added some logging which indicated the cache delete was not always being fired. Looks like the issue is that there is a slight timing issue between when the timeout is called and oldRecord.expire < Date.now().

In this case, the timeout fires, but oldRecord.expire < Date.now() is still true, so the cache object is not deleted. Timeout has been cleared, so the code won't get called again.

I forked node-cache and hacked in the line:

if (!isNaN(record.expire)) {
     record.timeout = setTimeout(function() {
+      delete record.expire;
       exports.del(key);

While this solves the issue for my case, e.g. timeout always wins, i wouldn't know if it has unintended consequences, or what would be the better way to fix this.

The more direct approach is to assume if export.del is called, the user really wants to delete, regardless of expiration.

On second review, additionally, maybe the logic was supposed to be ">" not "<"? I assume the intent was to prevent deletion if the expiration date has not yet been reached?

if (!isNaN(oldRecord.expire) && oldRecord.expire > Date.now()) {
  canDelete = false;
}

[VictorioBerra]

+1

[edeliu2000]

+1. This issue is especially obvious on large scale. I am running a 20 nodejs cluster and am using this library for in memory caching. The memory leak is obvious at such scale and causing crashes aver few hours. What I did is always delete on calling the del(), method regardless of the record timestamp.

[edeliu2000]

The memory leak issue is fixed btw. after making the change on the delete method and so far I don't see any side effects.

[heady]

@kavika-1 +1 the comparison should be reversed. The oldRecord.expire won't be less than Date.now since it equals Date.now + time. It will be greater than Date.now until time=0 at which point cache.del should pass and canDelete=true. I'd been testing a similar bug locally and this solved the cache.del('key') issue.

Adding a PR to resolve.

[heady]

Withdrawing the PR due to a broken test, will resubmit once something better matches.

[NeoLegends]

Why does it prevent deletion anyway? This is a cache, not a "store-at-least-until"-object. It makes manual cache invalidation completely impossible without modifying the source.

[justincy]

It's actually not trying to prevent deletion. It's treating expired objects as though they don't exist. That's the cause of the memory leak. When the expire timeout fires, exports.del(key) is called, but the object has expired so the delete doesn't actually occur and thus remains in memory.

It can be fixed by having the expire timeout skip the expires timestamp check and just delete. I tried adding a failing test case but tests are using sinon fake timers which don't exhibit the normal ms drift in the firing of timeouts so this condition can't be replicated. Otherwise I would fix it.

[justincy]

I just submitted a PR to fix this. I was able to add a failing test case for this issue by temporarily disabling the fake timers.