OFBiz
  1. OFBiz
  2. OFBIZ-2882

EntityList cache clearing issues when removing generic entity via DelegatorImpl

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Invalid
    • Affects Version/s: Trunk
    • Fix Version/s: None
    • Component/s: framework
    • Labels:
      None

      Description

      For more information refer to http://ofbiz.135035.n4.nabble.com/EntityList-cache-issues-td206907.html

      Ran into some trouble when we turned out caching and started to dependent on the EntityList cache. The two problems were:

      1) When attempting to storeHook to the entityListCache or entityObjectCache via the Cache.remove method, the NEW entity was being passed into the OLD entity. This caused condition matching (in the list cache) to sometimes fail if a matched entity no longer matches do to it being modified.

      2) During the matching logic the EntityJoinOperator was incorrectly short circuiting. It was always checking if the lhs/rhs condition was true and if so returning the short-circuit value. A concrete example is as such – (A is funny) && (B is funny). The short-circuit value for this expression is "FALSE" which means that if the first expression is FALSE we short-circuit and return FALSE. What was happening was "if (A is funny) then return FALSE" rather then "if (A is funny == FALSE) then return FALSE".

      I have a patch in the works for both of these issues and will include a unit tester that illustrates the problems (pre-patch) and passes successfully (post-patch).

        Activity

        Bob Morley created issue -
        Hide
        Bob Morley added a comment -

        This file contains a new unit test class for cache and exposes the problem with flushing when an entity gets updated.

        Show
        Bob Morley added a comment - This file contains a new unit test class for cache and exposes the problem with flushing when an entity gets updated.
        Bob Morley made changes -
        Field Original Value New Value
        Attachment OFBIZ-2882_EntityCacheListTest.patch [ 12418405 ]
        Hide
        Bob Morley added a comment -

        Includes the fix as well as the entitytests.xml update that I had forgotten to include with the test in the previous file.

        Show
        Bob Morley added a comment - Includes the fix as well as the entitytests.xml update that I had forgotten to include with the test in the previous file.
        Bob Morley made changes -
        Attachment OFBIZ-2882_EntityCacheListFix.patch [ 12418412 ]
        Adam Heath made changes -
        Assignee Adam Heath [ doogie ]
        Hide
        Bob Morley added a comment -

        FYI – I have not updated the patch; but we did find a bug with this patch in our code. In Cache.java remove(GenericEntity) it was checking to see if it could get the oldEntity, but what it did not realize was that the oldEntity could actually exist but be set to the internal NULL. This is true on creates of brand-new entities.

        We changed the method to the following:

        public GenericValue remove(GenericEntity entity) {
        if (Debug.verboseOn()) Debug.logVerbose("Cache remove GenericEntity: " + entity, module);
        GenericValue oldEntity = entityCache.remove(entity.getPrimaryKey());

        if (oldEntity == null) {
        // Unable to find the old entity in the case, so we will create a version of the original entity to properly remove the items from the related caches
        GenericEntity origEntity = GenericEntity.createGenericEntity(entity);
        if (entity instanceof GenericValue)

        { origEntity.setFields(((GenericValue)entity).getOriginalDbValues()); }

        entityListCache.storeHook(origEntity, null);
        entityObjectCache.storeHook(origEntity, null);

        } else {
        if( !(oldEntity instanceof NULL ))

        { // Remove the old entity from the related caches entityListCache.storeHook(oldEntity, null); entityObjectCache.storeHook(oldEntity, null); }

        }

        return oldEntity;
        }

          • We basically added the instanceof NULL check before going to storeHook with an oldEntity that represents the NULL generic entity.
        Show
        Bob Morley added a comment - FYI – I have not updated the patch; but we did find a bug with this patch in our code. In Cache.java remove(GenericEntity) it was checking to see if it could get the oldEntity, but what it did not realize was that the oldEntity could actually exist but be set to the internal NULL. This is true on creates of brand-new entities. We changed the method to the following: public GenericValue remove(GenericEntity entity) { if (Debug.verboseOn()) Debug.logVerbose("Cache remove GenericEntity: " + entity, module); GenericValue oldEntity = entityCache.remove(entity.getPrimaryKey()); if (oldEntity == null) { // Unable to find the old entity in the case, so we will create a version of the original entity to properly remove the items from the related caches GenericEntity origEntity = GenericEntity.createGenericEntity(entity); if (entity instanceof GenericValue) { origEntity.setFields(((GenericValue)entity).getOriginalDbValues()); } entityListCache.storeHook(origEntity, null); entityObjectCache.storeHook(origEntity, null); } else { if( !(oldEntity instanceof NULL )) { // Remove the old entity from the related caches entityListCache.storeHook(oldEntity, null); entityObjectCache.storeHook(oldEntity, null); } } return oldEntity; } We basically added the instanceof NULL check before going to storeHook with an oldEntity that represents the NULL generic entity.
        Hide
        Bob Morley added a comment -

        NOTE: I think this patch is pretty important. I have re-packaged it and tested it again on Trunk, here are the details ... Only apply the two V2 patch files!!!

        This patch fixes a pretty serious caching issue. I just applied to two patches to trunk and made a few modifications based on the current state of trunk. Here are the changes:

        a) The unit tester was making use of reflection to get at the cache (which was not exposed at the time of coding). It turns out that there is a getCache on delegator that is now available so I removed the reflection code and used this.

        b) The last assert on the unit test was assuming that the result of the cache (after clear would be null); I felt this was too close to the implementation so I eased this assert up to check for empty (as long as we do not have a cache hit on the removed entity we are ok)

        The changes to the fix were:

        a) Half of the fix was related to fixing EntityJoinOperator which was not properly checking the short circuit. It appears Adam Heath has applied this fix so I removed it from this patch.

        b) There was an additional "GenericValue.NULL" check that was written in the comments but not put into the patch file. The new patch file has this in it.

        As before, I ran with the unit test that exposes the failure and then ran again with the patch applied which causes the unit tester to pass.

        Show
        Bob Morley added a comment - NOTE: I think this patch is pretty important. I have re-packaged it and tested it again on Trunk, here are the details ... Only apply the two V2 patch files!!! This patch fixes a pretty serious caching issue. I just applied to two patches to trunk and made a few modifications based on the current state of trunk. Here are the changes: a) The unit tester was making use of reflection to get at the cache (which was not exposed at the time of coding). It turns out that there is a getCache on delegator that is now available so I removed the reflection code and used this. b) The last assert on the unit test was assuming that the result of the cache (after clear would be null); I felt this was too close to the implementation so I eased this assert up to check for empty (as long as we do not have a cache hit on the removed entity we are ok) The changes to the fix were: a) Half of the fix was related to fixing EntityJoinOperator which was not properly checking the short circuit. It appears Adam Heath has applied this fix so I removed it from this patch. b) There was an additional "GenericValue.NULL" check that was written in the comments but not put into the patch file. The new patch file has this in it. As before, I ran with the unit test that exposes the failure and then ran again with the patch applied which causes the unit tester to pass.
        Bob Morley made changes -
        Gavin made changes -
        Workflow jira [ 12474507 ] OFbiz Workflow [ 12507250 ]
        Bob Morley made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Jacques Le Roux added a comment -

        What is the state of this issue?

        Show
        Jacques Le Roux added a comment - What is the state of this issue?
        Hide
        Bob Morley added a comment -

        I believe the V2 set of patches just need a review and can be committed. You can apply just the unit tester first to expose the problems (perhaps do a little digging if you wish) and then apply the fix and the unit test will work properly.

        Show
        Bob Morley added a comment - I believe the V2 set of patches just need a review and can be committed. You can apply just the unit tester first to expose the problems (perhaps do a little digging if you wish) and then apply the fix and the unit test will work properly.
        Hide
        Jacques Le Roux added a comment -

        Thanks Bob,

        Hopefully Adam will get a chance to had a look, else I will try to review at least...

        Show
        Jacques Le Roux added a comment - Thanks Bob, Hopefully Adam will get a chance to had a look, else I will try to review at least...
        Hide
        Jacques Le Roux added a comment -

        Hi Adam,

        Did you get a chance to have a look at this issue?

        Show
        Jacques Le Roux added a comment - Hi Adam, Did you get a chance to have a look at this issue?
        Jacques Le Roux made changes -
        Assignee Adam Heath [ doogie ] Jacques Le Roux [ jacques.le.roux ]
        Jacques Le Roux made changes -
        Status Patch Available [ 10002 ] In Progress [ 3 ]
        Jacques Le Roux made changes -
        Description For more information refer to http://www.nabble.com/EntityList-cache-issues-...-to25179637.html

        Ran into some trouble when we turned out caching and started to dependent on the EntityList cache. The two problems were:

        1) When attempting to storeHook to the entityListCache or entityObjectCache via the Cache.remove method, the NEW entity was being passed into the OLD entity. This caused condition matching (in the list cache) to sometimes fail if a matched entity no longer matches do to it being modified.

        2) During the matching logic the EntityJoinOperator was incorrectly short circuiting. It was always checking if the lhs/rhs condition was true and if so returning the short-circuit value. A concrete example is as such -- (A is funny) && (B is funny). The short-circuit value for this expression is "FALSE" which means that if the first expression is FALSE we short-circuit and return FALSE. What was happening was "if (A is funny) then return FALSE" rather then "if (A is funny == FALSE) then return FALSE".

        I have a patch in the works for both of these issues and will include a unit tester that illustrates the problems (pre-patch) and passes successfully (post-patch).
        For more information refer to http://ofbiz.135035.n4.nabble.com/EntityList-cache-issues-td206907.html

        Ran into some trouble when we turned out caching and started to dependent on the EntityList cache. The two problems were:

        1) When attempting to storeHook to the entityListCache or entityObjectCache via the Cache.remove method, the NEW entity was being passed into the OLD entity. This caused condition matching (in the list cache) to sometimes fail if a matched entity no longer matches do to it being modified.

        2) During the matching logic the EntityJoinOperator was incorrectly short circuiting. It was always checking if the lhs/rhs condition was true and if so returning the short-circuit value. A concrete example is as such -- (A is funny) && (B is funny). The short-circuit value for this expression is "FALSE" which means that if the first expression is FALSE we short-circuit and return FALSE. What was happening was "if (A is funny) then return FALSE" rather then "if (A is funny == FALSE) then return FALSE".

        I have a patch in the works for both of these issues and will include a unit tester that illustrates the problems (pre-patch) and passes successfully (post-patch).
        Hide
        Jacques Le Roux added a comment -

        I close this issue because the fixing patch is now invalid. But I applied the test patch and it works. I believe it's incomplete because of OFBIZ-5192. Also I'm not sure of it's path, there is already EntityTestSuite.java where it could fit. It's minor but can be discussed before being commited

        Show
        Jacques Le Roux added a comment - I close this issue because the fixing patch is now invalid. But I applied the test patch and it works. I believe it's incomplete because of OFBIZ-5192 . Also I'm not sure of it's path, there is already EntityTestSuite.java where it could fit. It's minor but can be discussed before being commited
        Jacques Le Roux made changes -
        Status In Progress [ 3 ] Closed [ 6 ]
        Resolution Invalid [ 6 ]
        Hide
        Adrian Crum added a comment -

        I agree this issue can be closed. I came to the same conclusion as Bob and bypassed storeHook altogether in revision 1476296.

        Show
        Adrian Crum added a comment - I agree this issue can be closed. I came to the same conclusion as Bob and bypassed storeHook altogether in revision 1476296.

          People

          • Assignee:
            Jacques Le Roux
            Reporter:
            Bob Morley
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development