Cache API not thread safe and inconsistent with context parameter

Description

This is somewhat related to IDEMPIERE-4286.

In adempiere times a practice was introduced to add getters from cache with trx, some of those getter methods even change the trx of the object recovered from cache (not verifying if the object is fresh as in theory it can be reading a stale object different from the one in DB).

Although the incidence of problems caused by this is low (and probably this is why this has not been reported or tackled) - in heavy usage environments the occurrences must be more frequent, and probably reviewed like different errors (like working on a closed transaction, or writing stale data to the database, or db locks because of competing transactions).

This ticket is to evaluate the possibilities and try to get a good solution for this.

has suggested that cache objects must be read-only.
Maybe we could implement a way to enforce that in code - add some isFromCache or isReadOnly property in PO object and forbidding to save/delete those objects.

That change can impact a lot of core processes and plugins, so it would require extensive tests. That means the developer must be very conscious about when requiring a r/o object is preferably to use cache (performance wise), but when requiring a r/w object must read it from database probably using a constructor, and maybe even as a best practice we can enforce the usage of transactions on r/w objects.

WDYT?

Additional Notes from :

PO Cache is usually implemented using the pattern of “public static ModelClass ModelClass.get(Properties ctx, int id)” or “public static ModelClass ModelClass.get(Properties ctx, int id, String trxName)”

Issues with the current implementation:

  1. Inconsistent handling of the “Properties ctx” method. When PO is retrieve from cache, the ctx parameter is not use. Consequently the return PO might not have the same ctx as the ctx parameter.

  2. Since PO is mutable, the static PO cache is not thread safe. Multiple thread/session can get the same PO instance from cache and making concurrent changes to it.

  3. If the ctx parameter is a plain Properties object instead of an instance of ServerContextPropertiesWrapper, keeping it in cache is memory intensive and a security risk (possibilities to get PO with different user’s session context contents).

  4. If multiple thread perform write transaction with the same PO instance from cache, dead lock can happens. This is not usually the case in current code base but the current cache API allows that.

Environment

None

Activity

Show:
Carlos Ruiz
September 14, 2020, 8:44 PM

Case 4 -> solved
Case 6 -> solved

Case 7

  • Form Reset Password -> PO is Immutable: org.compiere.model.MUser

Case 8

  • Created a requisition for 1 Seeder

  • Complete -> PO is Immutable: org.compiere.model.MProcess

Carlos Ruiz
September 18, 2020, 6:04 PM

Hi , about the last commit:

> - remove usage of cache for transaction table (rfq, invoice, inventory).

I think is not a good idea to remove the cache from those classes, there are cases where performance can be improved reading for example invoices from cache instead of DB, when you just need some data from the invoice/lines, but don't need to write anything back.

Probably there are not examples in core, but in plugins is possible to find processes that perform heavy operations and needs those read-only cached. If the immutable control is already implemented, then I think is harmless to keep the read-only cache in case is needed.

Regards,

Carlos Ruiz

Heng Sin Low
September 18, 2020, 10:01 PM

Hi Carlos,

  1. It is hard and tricky to make a transaction table PO immutable.

  2. A global system cache is only useful if you have to read a record repeatably and that’s not the case for transaction table.

  3. If you have to process/read a large number of transactional records, for e.g invoices in a process, you should implement that cache in the process itself and not depends on a system level, global cache - that cache should be throw away from memory since it is not going to be very useful for others.

Regards,

Low

Carlos Ruiz
September 19, 2020, 9:25 AM

OK for me then - if we see the need it can be added later, I think the impact of this ticket can be huge in plugins and the sooner community can start testing it the better.

Carlos Ruiz
October 16, 2020, 4:50 PM

Detected a new problem:

Test case:

  • Log in as SuperUser with GardenWorld User Role

  • Change any record (for example Freight Category Air)

  • Log out

  • Log in as SuperUser with GardenWorld Admin Role <- different role

  • Try to do any change on the same record

  • Result:

Assignee

Heng Sin Low

Reporter

Carlos Ruiz

Tested By

None

Fix versions

Priority

Minor
Configure