[jbosscache-dev] ClusteredCacheLoader deadlocks and JBCCACHE-1103

[Bela Ban]

OK, yes, having remote puts and CCL'induced gets not block on the same 
lock is certainly a much better solution.

What will also help is JGroups 2.5 *should* we still encounter a deadlock.

BTW: I assume using the Multiplexer here is out of the question ? You 
could use a separate stack in the Multiplexer (e.g. "udp-repl-sync") 
which is only used by the CCL, and has no FC in it, and everyone else 
uses the async stack.

But then again, there was a reason we didn't configure the Multiplexer 
to be the defaut in 2.4...

Manik Surtani wrote:
>
> On 19 Jun 2007, at 09:04, Bela Ban wrote:
>
>> Is this going to solve JBCACHE-1103 ? I mean, +1 for the lock 
>> refactoring, sounds like it needs to be done anyway. But, because you 
>> have to be thread safe at one point, aren't you deferring the issue 
>> to a later stage ? At one point you have to acquire a lock to prevent 
>> concurrent loading, and I'm afraid we'll have the same issue.
>>
>> But, wait, if you defer lock acquisition till after the synchronous 
>> cluster method call, you should be fine. Is this what you have in mind ?
>
> There are 2 locks here that need to be done.  One lock is on the Node, 
> via the lock interceptor, which happens *after* the cluster method 
> call.  This is not the problem.
>
> We also had a level of sync in the cache loader interceptor to prevent 
> the same Fqn being loaded twice from the CL (or a simultaneous load 
> and store, etc).  This bit was the problem, since this sync had to 
> happen *before* any loading was done by the CL.
>
> Regarding the other CL impls, some of them already were threadsafe 
> (BDBJE, Jdbm) and some weren't (JDBC, File). I created a StripedLock 
> class which uses a set of ReentrantReadWriteLocks applied to the Fqn 
> in question, and the JDBC and File CLs now use this to achieve thread 
> safety (more efficient than synchronized blocks and better mem usage 
> than one lock per Fqn - usual benefits of lock striping).
>
> You're right that the improvements in locking here are not quite 
> central to solving 1103, but somewhat related.
>
> The real solution to 1103 is in the CCL - remotely originating put()'s 
> shouldn't need a load (and hence not need to wait on the Fqn being 
> loaded) *only if* we're talking about the CCL, since CCLs don't 
> perform a corresponding store (they're read-only).  Therefore this 
> logic cannot be in the CLI as it doesn't apply to all CL impls.
>
>
>>
>> On the same topic: I looked into interrupting clustered method calls 
>> in JGroups (http://jira.jboss.com/jira/browse/JGRP-533), and came to 
>> the conclusion that it isn't feasible. When a thread is blocked on 
>> FC.down(), waiting for credits, then I can interrupt it, but it will 
>> go right back in its loop waiting for credits, therefore blocking 
>> again. Unless I change the loop condition (running & 
>> waiting-for-credits), the thread will always block if there aren't 
>> enough credits available.
>>
>> Thoughts ?
>>
>>
>> Manik Surtani wrote:
>>>
>>>
>>> Ok, I spent a bit of time investigating this, and the original sync 
>>> blocks in the CacheLoaderInterceptor, superceded by the 
>>> synchronization on an Fqn in subsequent versions are all hacks to 
>>> get around the fact that some cache loader implementations 
>>> themselves aren't thread safe.
>>>
>>> I wrote a test to check the thread safety of cache loader impls, and 
>>> most of what we have fail.  I patched the DummyInMemoryCacheLoader 
>>> to be thread safe (a lot of synchronization within the impl) and 
>>> removed the unnecessary locks in the interceptors and things work 
>>> fine.  I tried this with some of the few thread-safe cache loader 
>>> impls we have (BDBJE, for example) and this worked well as well.
>>>
>>> So what I propose is this:
>>>
>>> 1) Patch CLI to NOT use the Fqn locks (in BaseCacheLoaderInterceptor)
>>> 2) Make sure we acquire locks (lock() call up the interceptor stack) 
>>> *before* attempting to load node, but after creating temp node
>>> 3) Make sure cache loader impls are thread safe
>>> 4) Add thread safety test to CacheLoaderTestBase
>>> 5) Patch loaders that aren't threadsafe and fail test in (4) - 
>>> FileCacheLoader, JDBCCacheLoader, etc.
>>>
>>> I'm going to do this on my local checkout of HEAD and run the 
>>> regression tests overnight.  If this works, I suggest backporting 
>>> this to 1.4.x and regressing there.
>>>
>>> WDYT?
>>
>> -- 
>> Bela Ban
>> Lead JGroups / JBoss Clustering team
>> JBoss - a division of Red Hat
>
> -- 
> Manik Surtani
>
> Lead, JBoss Cache
> JBoss, a division of Red Hat
>
>
>

-- 
Bela Ban
Lead JGroups / JBoss Clustering team
JBoss - a division of Red Hat