polygene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Paul Merlin <p...@nosphere.org>
Subject Re: JSONEntityState internal clone logic
Date Mon, 27 Jul 2015 15:14:15 GMT
Guys,

I reviewed JSONEntityState cloning and Tibor catched an obvious mistake.

The cloneStateIfGlobalStateLoaded() method of JSONEntityState is called
before any change is made to the entity state.

This method starts with the following check:

if( isStateNotCloned() )
{
    return;
}
// Do the clone

And the (private) isStateNotCloned() method returns:
status == EntityStatus.LOADED

IOW, the code says that it will clone state if status is loaded, but it does the opposite!

Corresponding JIRA issue: https://issues.apache.org/jira/browse/ZEST-107.


Rewriting the cloneStateIfGlobalStateLoaded() method, as Tibor suggested, fixes the issue:

if( status != EntityState.LOADED )
{
    return;
}
// Do the clone

I removed the isStateNotCloned() method BTW as it was not used elsewhere and the code is simpler
without it.


Moreover, I reworded the test case Tibor provided so it fits in AbstractEntityStoreTest as
this:

@Test
public void givenEntityStoredLoadedChangedWhenUnitOfWorkDiscardsThenDontStoreState()
    throws UnitOfWorkCompletionException
{
    UnitOfWork unitOfWork = module.newUnitOfWork();
    try
    {
        String identity = createEntity( unitOfWork ).identity().get();
        unitOfWork.complete();

        unitOfWork = module.newUnitOfWork();
        TestEntity entity = unitOfWork.get( TestEntity.class, identity );
        assertThat( entity.intValue().get(), is( 42 ) );
        entity.intValue().set( 23 );
        unitOfWork.discard();

        unitOfWork = module.newUnitOfWork();
        entity = unitOfWork.get( TestEntity.class, identity );
        assertThat( entity.intValue().get(), is( 42 ) ); // Fail here!
    }
    finally
    {
        unitOfWork.discard();
    }
}

This test pass when no CachePool service is assembled.

Without the fix described above, and as soon as a CachePool service is used by (visible to)
the EntityStore, this test fail: the cached version is returned and the discarded change (intValue
from 42 to 23) is loaded when it shouldn't be.

I then added some AbstractEntityStoreWithCacheTest in core/testsupport to reuse all tests
from AbstractEntityStoreTest adding a MemoryCachePool to the assembly (overridable). And implemented
it for all of our EntityStores that suppots the Cache SPI (all - [Preferences, SQL]).

So far so good, all SDK tests passes with theses changes.

I did that work in a branch, merged into develop as a single commit, "à la" gitflow, so we
can revert all this easily if needed and keep the complete history.

Any comments?

/Paul

BTW, see https://issues.apache.org/jira/browse/INFRA-8434 for how to close github pull requests
from asf repositories



Tibor Mlynarik a écrit :
> yes, test in pull request.
>
> cheers, 
>
> 	Tibor
>
>
> On Jul 24, 2015, at 7:01 AM, Niclas Hedhman <niclas@hedhman.org> wrote:
>
>> The "this testcase" is referring to the pull request? I'll take a look a
>> bit later.... Checking the "Release" at the moment.
>>
>> Cheers
>> Niclas
>>
>> On Fri, Jul 24, 2015 at 12:14 PM, Tibor Mlynarik <tibor.mlynarik@gmail.com>
>> wrote:
>>
>>> Hi Niclas,
>>>
>>>> The "isStateNotCloned()" fix seems legit and a testcase for that would be
>>>> awesome (can't figure one out in my at this early hour)
>>> this testcase should be showing that "isStateNotCloned()" is broken.
>>> Change to entity in uow3, which is discarded, is visible to others, as
>>> cloning is not working.
>>>
>>> account3.balance().set( BigDecimal.TEN);
>>> uow3.discard() ;
>>>
>>> on other side, clone is executed with each set to entity, what is probably
>>> only unnecessary performance penalty.
>>>
>>>> As for the testcase, the bug that it exposes is that it doesn't prevent
>>> you
>>>> from using the "Account account1" references outside the UoW it was
>>> created
>>>> in.
>>> I see your point that accessing entity fetched from other uow ( closed
>>> one) should be prohibited.
>>> But maybe accessing just identity value should be allowed.
>>> But this is not what I wanted to report.
>>>
>>> thanks ,
>>>
>>>        Tibor
>>>
>>> On Jul 24, 2015, at 3:50 AM, Niclas Hedhman <niclas@hedhman.org> wrote:
>>>
>>>> I have not checked the code yet, but the clone is supposed to be there to
>>>> provide UoW isolation, i.e. a value pulled out of the cache, and modified
>>>> (any of the set methods) must not be visible into the cached state until
>>>> the UoW completes, at which time I presume the entity state is replaced
>>> in
>>>> the the cache.
>>>>
>>>> As for the testcase, the bug that it exposes is that it doesn't prevent
>>> you
>>>> from using the "Account account1" references outside the UoW it was
>>> created
>>>> in. That is the bug, and I would be happy to look into that (perhaps for
>>>> 3.0 just in case you depend on this fault behavior). If identity is used,
>>>> it is more likely working as expected.
>>>>
>>>>
>>>> The "isStateNotCloned()" fix seems legit and a testcase for that would be
>>>> awesome (can't figure one out in my at this early hour)
>>>>
>>>>
>>>> Thanks, Tibor
>>>>
>>>>
>>>> On Fri, Jul 24, 2015 at 12:31 AM, Tibor Mlynarik <
>>> tibor.mlynarik@gmail.com>
>>>> wrote:
>>>>
>>>>> Hi Paul,
>>>>>
>>>>> I have prepared test case to demonstrate issue.
>>>>> Problem is if there is enabled cache in json based entity store.
>>>>> Entities could be in wrong state.
>>>>>
>>>>> see here :
>>>>> https://github.com/apache/zest-qi4j/pull/2
>>>>>
>>>>> to make test success, fix :
>>>>>
>>>>> JSONEntityState
>>>>>   void cloneStateIfGlobalStateLoaded()
>>>>>   {
>>>>>       if( ! isStateNotCloned() )
>>>>>
>>>>>
>>>>> there is one more fix needed in JSONMapEntityStoreMixin#fetchCachedState
>>>>> method .
>>>>>
>>>>> Note: I was not able to configure entity store with cache service in
one
>>>>> layer ,
>>>>> probably because of circular dependency: store uses cache, cache need
>>>>> store for config .
>>>>>
>>>>> cheers,
>>>>>
>>>>>       Tibor
>>>>>
>>>>>
>>>>> On Jul 23, 2015, at 1:49 PM, Paul Merlin <paul@nosphere.org> wrote:
>>>>>
>>>>>> Hi Tibor,
>>>>>>
>>>>>> Tibor Mlynarik a écrit :
>>>>>>> Hi,
>>>>>>>
>>>>>>> I have noticed code in JSONEntityState that seems suspicious
for me.
>>>>>>>
>>>>>>> In method JSONEntityState#cloneStateIfGlobalStateLoaded
>>>>>>> if parts are swapped.
>>>>>>>
>>>>>>> I suppose that by global state is meant copy taken from cache
that is
>>>>> shared across UoWs.
>>>>>>> And before first entity change own copy for UoW should be cloned.
>>>>>>>
>>>>>>> But as it is now, cloning is not done at first change and redundantly
>>>>> executed with each entity change.
>>>>>>> Also maybe whole cloning could be avoided if cache is not used.
>>>>>>>
>>>>>>> Or am I wrong in understanding of purpose of this state clone
?
>>>>>> IIRC this is the sole purpose of this state clone.
>>>>>> As you pointed out there may be room for improvements here.
>>>>>>
>>>>>> Niclas, WDYT?
>>>>>>
>>>>>> /Paul
>>>>>>
>>>> --
>>>> Niclas Hedhman, Software Developer
>>>> http://zest.apache.org - New Energy for Java
>> -- 
>> Niclas Hedhman, Software Developer
>> http://zest.apache.org - New Energy for Java
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message