3.1.0 - dangerous shared objects for in memory cache

[ankopainting]

Hey,

The fix for #103 is not correct.

Basically the problem with the in memory cache's default behaviour WAS that any fetches from the cache were references to the same memory element, so mutations on any of them would be silently updating the cached value.

The fix above deep clones the object on set, which just hides the problem one fetch longer. It needs to do the clone ON GET so that each object returned is unique. This is other other backends work, eg. redis.

Here is some code to prove my problem;

const CacheManager = require('cache-manager')
let cache = CacheManager.caching({store: 'memory', max: 10, ttl: 10});

async function test(cache) {
  let a = await cache.wrap("string", () => {return {name: "frank", age: 6}});
  console.log(a);
  a.name = "bob"
  let b = await cache.wrap("string", () => {return {name: "frank", age: 6}});
  console.log(b);
  b.name = "bob2";
  let c = await cache.wrap("string", () => {return {name: "frank", age: 6}});
  console.log(c);
}

test(cache);

This should return the same object, {name: "frank", age: 6} all 3 times.

instead on v3.1.0 i get;

{ name: 'frank', age: 6 }
{ name: 'frank', age: 6 }
{ name: 'bob2', age: 6 }

The first one is the original version (eg. from the db or whatever populates the cache)
The second one is the deep clone you do on set, it's fetching from the cache
The third one is getting mutated!

This code
https://github.com/BryanDonovan/node-cache-manager/blob/master/lib/stores/memory.js#L56-L58

needs to be in the get :)

Thanks

[BryanDonovan]

I think people need to stop mutating things and clone the objects themselves to be safe.
If we move the clone to the get, you'll get the same problem, but it will occur earlier in your example, right? Cloning on set and get might be the only option, but that also seems odd.

I shouldn't have ever created the memory store. It was meant to be an example and I assumed most people would use a real cache and/or write their own stores. Too late now I guess. :)

[ankopainting]

Yep! you're right, you'd need it at both ends. Sorry that was my mistake.

Even if you avoid mutation (and i can agree it's bad practice) the other challenge if you don't clone is when debugging memory leaks - wondering why stuff referenced by the cache hasn't been garbage collected.

I'm inheriting our code that uses this library, but the original code mostly uses it for the tiered caching use case, where it fetches from memory and falls back to redis etc.. I think there is a good reason to have memory and redis to behave the same, in this case otherwise problems are very tricky to debug.

I'm sorry to be causing you pain, i have fixed my implementation i just wanted to feedback a problem so other people don't run into it too.

[BryanDonovan]

Oh, it's no pain from you :). Thanks for catching this. I didn't think of this case. When I made the "fix" I had originally done the clone on fetch, but my tests failed, so I moved the clone to be during set.

At this point, I should probably add the clone during fetch by default, and add another param that lets the user disable that. In retrospect, I probably should have just told people to clone the data themselves or write their own memory store, but it's too late for that.

I don't have much time to get to this in the next several days, but hopefully next week. A pull request would be appreciated if you have the time. The relevant tests are in the memory_store.unit.js file.