Neat! Just tried quickly and seems to work on ISPN test... I'll see if it works in 2LC and if so I'll send a PR.
We can then inspect where else this check might be needed. Cheers, -- Galder Zamarreño Infinispan, Red Hat > On 27 Feb 2017, at 14:01, Radim Vansa <rva...@redhat.com> wrote: > > Hi Galder, > > another solution came upon my mind (actually after finding this SO > question [1]): we could check if > > (this.key == key || this.key.equals(key)) > > in SingleKeyNonTxInvocation context instead of just > > this.key.equals(key) > > and that could do the trick. Plus, such check maybe needed in couple of > other places, haven't tried that. > > Seems that HashMap already does so, so the other types of contexts > should work OOTB. > > Radim > > [1] > http://stackoverflow.com/questions/13521184/equals-returns-false-yet-object-is-found-in-map > > On 02/20/2017 05:22 PM, Radim Vansa wrote: >> 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 <rva...@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 <rva...@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 <rva...@redhat.com> >>>>>> JBoss Performance Team >>>> -- >>>> Radim Vansa <rva...@redhat.com> >>>> JBoss Performance Team >>>> >> > > > -- > Radim Vansa <rva...@redhat.com> > JBoss Performance Team > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev@lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev _______________________________________________ infinispan-dev mailing list infinispan-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev
