Skip to content
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

Memory backend leaks references to its contents #28

Closed
fdegiuli opened this issue May 20, 2016 · 2 comments
Closed

Memory backend leaks references to its contents #28

fdegiuli opened this issue May 20, 2016 · 2 comments

Comments

@fdegiuli
Copy link
Contributor

fdegiuli commented May 20, 2016

When using the 'memory' backend, getting a value returns a reference to the object in the backend map. Mutating the returned object affects the contents of the map, and therefore all subsequent get calls.
Since this obviously can't happen with memcached, I figure it's a bug.

E.g.:

var c = cached('test', { backend: { type: 'memory' }});
cache.set('a', {a: [ { a:1}, {a: 2}, name: 'a'], console.log);
c.get('a', console.log);
// > null { a: [ { a: 1 }, { a: 2 } ], name: 'a' }
c.get('a', function(err, data) { data.a = 'fail' });
c.get('a', console.log);
// > null { a: 'fail', name: 'a' }

Obviously a deep copy for every get operation has some performance implications, so I wanted to get your opinion before throwing you a PR. The way I see it there are three options:

  1. Keep the behavior, document it (in bold and underline)
  2. Always deep copy the value before returning it
  3. Add a config parameter to the memory backend to let the user choose whether to copy or not
@jkrems
Copy link
Contributor

jkrems commented May 20, 2016

We did stringify initially for the memory backend but it led to serious performance issues. So I'd definitely prefer just having a note in the docs. E.g. we use it in some places to store large Buffer instances where copy would be pretty expensive (and undo a lot of the advantages of caching in the first place).

In my experience copy on read is often a suboptimal solution and using immutable data structures (even just Object.freeze) is more efficient. But I wouldn't want to make cached opinionated on how apps handle immutability. That would be my argument against option 3.

@fdegiuli
Copy link
Contributor Author

I made you this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants