MLocation.get cache all MLocations

Revision: 12346
http://adempiere.svn.sourceforge.net/adempiere/?rev=12346&view=rev
Author: teo_sarca
Date: 2010-05-17 15:08:56 +0000 (Mon, 17 May 2010)

Log Message:
-----------
BF [ 3002736 ] MLocation.get cache all MLocations
Link to SF Tracker: http://sourceforge.net/support/tracker.php?aid=3002736

Modified Paths:
--------------
trunk/base/src/org/compiere/model/MLocation.java

Teo and other peer reviewers.

I think a better approach could be:

| MLocation retValue = (MLocation) s_cache.get (key);
| if (retValue != null) {
| if (retValue.get_TrxName() != trxName)
| retValue.set_TrxName(trxName);
| return retValue;
| }

I mean - if the trx is different than asked, assign it.
This could be the general approach for all the classes with getters.

Using cache just for non-trx objects practically make the cache useless.

WDYT?

Regards,

Carlos Ruiz

Hi Carlos,

Hmmm, this is a very sensible and serios subject. I am not agree to cache objects that belong to a transaction to a global cache (like in this case, a static cache object which is accessible to all system, all threads etc).

Also, having our current cache system, I am against using the PO.set_TrxName method because can easily conduct you to unexpected results. I even wanted to propose to set that method private and let the object stick with the transaction where it was loaded. More, think that a static cache is accessible to more then one thread, more then one transactions that are running and the stored objects are not immutable, nor cloned so in this case you will be easily change the trxName of a PO object that is used in other transaction, right in that moment.

Now, to be able to do caching per transaction, I think we need to introduce a new caching object (e.g. POTrxCache) which should do following:
* be able to return the needed PO by Key and by transaction name
* listen to Trx rollback and commit events, and should clear the cache for that trxName; a nice feature will be in case of commit, to "move" the cached POs from transaction cache segment to out of transaction cache segment (trxName = null).
Maybe is better to reevaluate some already build caching systems and use one of them, instead of reinventing the wheel.

> Using cache just for non-trx objects practically make the cache useless.
At this moment, I think that more than 80% of the caching objects are used out of transaction, so i doubt is useless.

WDYT?

Best regards,
Teo Sarca

Teo, your arguments sound correct, specially the danger of accesing the same object from different threads.

Question to Heng Sin - in zkwebui different sessions share the same cache? Or there is a cache per session?

If the answer is sessions share the same cache, then we're in serious risk and probably we need to follow the approach implemented by Teo on rev 12346 for all cached objects.

*****
2nd
The other issue I would like to comment is - if we would follow the approach recommended by Teo (I would like to have Heng Sin review here as System Architect) instead of changing specifically the class we can implement the validation in method CCache.put, something like adding these lines on the method:
if (value instanceof PO) return value;

So, we cover all the classes (core and extensions) that implemented cache.

*****
3rd
>> Using cache just for non-trx objects practically make the
>> cache useless.
> At this moment, I think that more than 80% of the caching
> objects are used out of transaction, so i doubt is useless.

That's my concern - using cache just for out-of-trx objects probably will push a non-best-practice - read-only cached objects must be read without transaction - well, probably is not a bad practice in the end.

Regards,

Carlos Ruiz

Hi Carlos,

No, zk doesn't do cache per session, it is the same global cache from base.

Agree that cache should only be used for out of trx PO. However what Teo implemented here is only a partial solution as there is nothing there will stop people from getting the cached object, set trx name to it and used it to update the db. I guess probably a more robust approach is not to cache the PO but to cache a immutable copy of it ( ie, an immutable wrapper of the real po ).

Regards,
Low

Integrated partial solution into release with revision 12529.

Keeping this open as the problem is general.