memoized values open to corruption, clone option could be useful? about memoizee HOT 7 CLOSED

I understand the concern, and I think clone option is acceptable for that.

Let me sleep with it, and most likely tomorrow I'll address it. It is simple to integrate.

from memoizee.

@bojand Can you also describe your use case? Why you do not copy the object on your own before modyfing it?

from memoizee.

Basically I'm experimenting with memorization to briefly (right now i'm testing with maxAge of 1 s) cache some db calls that may happen in frequent succession on some requests, to optimize some performance. Something similar conceptually would be (I am simplifying the situation for brevity):

Let's say you have expensive db function:

getUserFromDB(id, fb)

and you memoize the result for 1 s max because it could potentially be called multiple times in succession for a single user id for a duration and processing of some request.

And then you have some request:

// in some middleware use getUserFromDB to fetch a user from db to check request permissions, and do some other stuff
// this would cache the resulting user for the given id for 1 s

in some route handler:

getListOfUsersFromDB(listOfUserIds, function(err, users) { 
   // ^ uses getUserFromDB and may get the cached data from invocation in middleware
   // depending on params
  async.map(users, function (u, cb) {
     getAddressFromDB(u.address, function(err, addr) {
       u.address = addr; // corrupts the cache :S
       cb(null, u);
     });
  }, function (err, results) {
     return res.send(200, results); // express response
});

Basically fetches a list of users and populates the address (which would be originally a string id) with actual address documents, and sends the result. Let's say getAddressFromDB expects the address id as parameter. If I make 2 of these requests quickly enough, cache could still be present and the address will already be populated in the user objects. And then that causes an issue for the getAddressFromDB. Perhaps this doesn't really show the necessity for memoization, but this is basically the example conceptually. I know there could be workarounds, such as I could clone the users variable returned by getListOfUsersFromDB before I call getAddressFromDB myself, but I don't want to be doing that on all requests, which a lot of the time is not necessary if the cache will not be present. Or I could make getAddressFromDB smarter but then it's not exactly modular, as why should that function be aware of just this one edge case because of "memoization of the user database fetch and the side-effect in this route"; when in other pieces of the system it works fine as intended.

I think a solution would be to just make getUserFromDB return the clone cached copy if it's there (given the clone parameter). And if no cache we'll go to the db and fetch things the hard way.

I hope I have explained it.

from memoizee.

I was thinking about it, and to mu understanding this issue should be solved outside of memoize.

It's rule of thumb not to modify objects that come from cache that we know can be provided also to other consumers. This rule spans onto many other patterns and doesn't apply only to memoization.

Within your function you're aware that you're using result that's memoized and can be used for other calls, so you should take necessary steps to not corrupt result. More convincing use case would be, if in some scenario you provide those results to functions that were not meant to use memoized results, and for valid reason modify arguments, but even then I would suggest to fix the issue through the adapter which will clone the object before providing it to the function.

Anyway, looking directly at your example, you may run getAddressFromDB only if u.address is not evaluated yet, and additionally memoize getAddressFromDb, so it's not called twice for same id. That will close all the leaks, and it might be better than object cloning.

from memoizee.

@medikoo I'm working on integrating memoizee into an already existing application, and finding that without a clone option a ton of code will need to be rewritten. Obviously if you know that you're pulling objects from cache, you shouldn't modify them without cloning. However, in my case, the application was written to pull objects direct from the server, and then the objects are modified and used as needed inside of the scope of that controller.

I think a clone option would be a very useful object to include. Or, if you're set on not allowing cloning, adding a freeze option to make the cache immutable would be helpful, too.

from memoizee.

@codyhazelwood what prevents you from adding simple adapter as:

var wrappedMemoizedFn = function (arg) {
  return copy(memoizedFn(arg));
};
// use wrappedMemoizedFn instead of memoizedFn
wrappedMemoizedFn();

from memoizee.

It's kind of messier than I would like, but here's what I'm using for now (with promises / Angular):

    function cloneMemoized(memoizedFunction) {
        return function () {
            return memoizedFunction().apply(this, arguments).then(
                function (memoizedData) { return _.cloneDeep(memoizedData); }, 
                function (err) { return $q.reject(err); }
            );
        };
    };

    // This is on a separate line so I can call memoizedGetClients.delete(args) or .clear()
    var memoizedGetClients = memoizee(..., {promise: 'then'});

    // Now getClients() returns a cloned 
    var getClients = cloneMemoized(memoizedGetClients);

from memoizee.