[infinispan-dev] GetKeyValueCommand NPE with CR1 in Hibernate 2LC - ISPN-7029

[Radim Vansa]

On 02/20/2017 03:52 PM, Galder Zamarreño wrote:
> I've just verified the problem and the NPE can be reproduced with Infinispan alone.
>
> More replies below:
>
> --
> Galder Zamarreño
> Infinispan, Red Hat
>
>> On 16 Feb 2017, at 10:44, Radim Vansa <rvansa at redhat.com> wrote:
>>
>> On 02/15/2017 06:07 PM, Galder Zamarreño wrote:
>>> --
>>> Galder Zamarreño
>>> Infinispan, Red Hat
>>>
>>>> On 15 Feb 2017, at 12:21, Radim Vansa <rvansa at redhat.com> wrote:
>>>>
>>>> On 02/15/2017 11:28 AM, Galder Zamarreño wrote:
>>>>> Hey Radim,
>>>>>
>>>>> Your changes in ISPN-7029 are causing issues with Hibernate 2LC.
>>>>>
>>>>> In particular [2]. Name.equals() always returns false, so it'll never be found in the context. So entry is null.
>>>> That's obviously a bug in the 2LC testsuite, isn't it?
>>> LOL, is it? You added the class and the javadoc clearly states that this entity defines equals incorrectly. You must have added it for a reason?
>>>
>>> https://github.com/hibernate/hibernate-orm/blob/master/hibernate-infinispan/src/test/java/org/hibernate/test/cache/infinispan/functional/entities/Name.java
>>>
>>> In any case, an object with an incorrect equals impl should not result in an NPE within Infinispan :|
>>>
>>>> Object used as @EmbeddedId needs to have the equals correctly defined. How else would you compare ids? I wonder how could that work in the past.
>>> ROFL... it's your baby so you tell me ;)
>> Okay, okay - I haven't checked the javadoc, so I just assumed that it's an oversight :)
>>
>>>>> Moreover, if EntryLookup.lookupEntry javadoc (and some implementations) can and do return null. Are you expecting that somehow that method will never return null?
>>>> With ISPN-7029 in, the entry should be wrapped in the context after EntryWrappingInterceptor if the key is in readCH, otherwise it should be null. In case that xDistributionInterceptor finds out that it needs to work on that value despite not being able to read it (e.g. in case that it's in writeCH during unfinished rebalance), it should wrap NullCacheEntry.getInstance() using EntryFactory. wrapExternalEntry. More info about the logic is in o.i.c.EntryFactory javadoc [3].
>>> Not sure I understand what you're trying to imply above... so, is lookupEntry() allowed to return null or not?
>> It is allowed to return null, but:
>>
>> 1. If the node is an owner according to readCH, the entry must be wrapped into context in EWI (possibly as NullCacheEntry).
>> 2. The code can reach the GKVC.perform() iff this node is an owner of given key.
>>
>> The problem here is that I've assumed that if the entry was wrapped, it can be fetched. With incorrectly defined equals, as we see here, this does not hold. So we can
>>
>> a) check if the entry is null and throw more explanatory exception - more code in perform()
>> b) do the lookup after wrapping and throw there - unnecessary map lookup for such annoying problem
>> c) ostrich approach
>>
>> I think that b) in assert could do, otherwise I'd suggest c)
> Hmmmmm, what about not throwing an exception at all?
>
> IOW, e.g. in SingleKeyNonTxInvocationContext.lookupEntry(), if the key is not equals to the one cached, but the cached one is NullCacheEntry, couldn't it return that instead of null? What I'm trying to achieve here is that if the equals is not correctly implemented, the get() returns null, rather than throwing an exception.
>
> We'd also need to verify whether other context implementations could deal with it.
>
> Wouldn't that be better :\ ? I can see the point of throwing an exception (other than NPE of course), but it feels a little abrupt... particularly since seems like previously we've just returned null in such situations.

You're right that Cache.get(x) should ideally return null.

InvocationContext.lookupEntry() returning NullCacheEntry and null has a 
different meaning, though:

* NullCacheEntry means that we are supposed to be able to read that 
entry (according to readCH) but we haven't found that in DC. This is 
used for example in CacheLoaderInterceptor, where we skip entries that 
aren't in the context as opposed to recomputing the hash of the key.
* null means that this entry cannot be read on this node because the 
segment this key belongs to is not in readCH, and therefore the command 
shouldn't get past distribution interceptor.

I don't mind changing the 'design' as long as it follows some simple 
rules. I don't see how returning NCE for *any* key could be incorporated 
into the rules, and how should that behave on multi-key contexts.

In all but distribution mode, during read the key must be always wrapped 
for read, because there's either no readCH (local) or replicated CH 
(repl/invalidation). But in invalidation/local mode we can't handle that 
in distribution interceptor as this is not present.

I would really prefer to see this handled in 
EntryWrappingInterceptor/EntryWrappingFactory rather than in each 
command - in #4564 [2] I wanted to narrow the possible situations to be 
handled in perform(), and other pieces (e.g. return handlers - see 
dataReadReturnHandler, that will throw NPE as well in TX RR right now). 
Note that some commands do the check ATM (e.g. functional commands) - I 
think that those are leftovers and should be gone.

I was about to suggest adding

if (!key.equals(key)) return null;

to all EWI.visitXXX methods as proper implemenations of equals should do the

if (other == this) return true;

anyway as a shortcut, so such check shouldn't affect performance. 
However, this is not as simple with functional commands which should 
operate on the null entry (on some node), we have to run these, and you 
can't even ignore the value, you need some entry in there.

I hate not finding my socks in the drawer where I definitely put those. 
This just breaks too many things :-(

Btw., it seems that SingleTxContextInvocationContext [1] will fail when 
the entry is loaded from cache store - this is the place where you could 
put the entry into invocation context twice. So the use case was 
somewhat broken before, too.


[1] 
https://github.com/infinispan/infinispan/blob/master/core/src/main/java/org/infinispan/context/SingleKeyNonTxInvocationContext.java#L114
[2] https://github.com/infinispan/infinispan/pull/4564


>
> Cheers,
>
>> Radim
>>
>>> To be more specific, SingleKeyNonTxInvocationContext.lookupEntry() can return null, so GetKeyValueCommand should be able to handle it? Or should SingleKeyNonTxInvocationContext.lookupEntry return NullCacheEntry.getInstance instead of null?
>>>
>>> To provide more specifics, SingleKeyNonTxInvocationContext has NullCacheEntry.getInstance in cacheEntry variable when it's returning null. Should it maybe return NullCacheEntry.getInstance instead of null from lookupEntry() ?
>>>
>>> Cheers,
>>>
>>>> Radim
>>>>
>>>> [3] https://github.com/infinispan/infinispan/blob/master/core/src/main/java/org/infinispan/container/EntryFactory.java
>>>>
>>>>> I'll create a JIRA to track all issues arising from Hibernate 2LC in a minute, but wanted to get your thoughts firts.
>>>>>
>>>>> Cheers,
>>>>>
>>>>> [1] https://issues.jboss.org/browse/ISPN-7029
>>>>> [2] https://github.com/hibernate/hibernate-orm/blob/master/hibernate-infinispan/src/test/java/org/hibernate/test/cache/infinispan/CacheKeysFactoryTest.java#L58
>>>>> --
>>>>> Galder Zamarreño
>>>>> Infinispan, Red Hat
>>>>>
>>>> -- 
>>>> Radim Vansa <rvansa at redhat.com>
>>>> JBoss Performance Team
>>
>> -- 
>> Radim Vansa <rvansa at redhat.com>
>> JBoss Performance Team
>>


-- 
Radim Vansa <rvansa at redhat.com>
JBoss Performance Team