<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 19, 2013 at 11:55 PM, Pedro Ruivo <span dir="ltr"><<a href="mailto:pedro@infinispan.org" target="_blank">pedro@infinispan.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im"><br>
<br>
On 03/19/2013 08:41 PM, Dan Berindei wrote:<br>
><br>
> On Mon, Mar 18, 2013 at 6:09 PM, Pedro Ruivo <<a href="mailto:pedro@infinispan.org">pedro@infinispan.org</a><br>
</div><div><div class="h5">> <mailto:<a href="mailto:pedro@infinispan.org">pedro@infinispan.org</a>>> wrote:<br>
><br>
> Hi all,<br>
><br>
> To solve ISPN-2808 (avoid blocking JGroups threads in order to allow to<br>
> deliver the request responses), I've created another thread pool to move<br>
> the possible blocking commands (i.e. the commands that may block until<br>
> some state is achieved).<br>
><br>
> Problem description:<br>
><br>
> With this solution, the new thread pool should be large in order to be<br>
> able to handle the remote commands without deadlocks. The problem is<br>
> that all the threads can be block to process the command that may<br>
> unblock other commands.<br>
><br>
> Example: a bunch of commands are blocked waiting for a new topology ID<br>
> and the command that will increment the topology ID is in the thread<br>
> pool queue.<br>
><br>
> Solution:<br>
><br>
> Use a smart command dispatcher, i.e., keep the command in the queue<br>
> until we are sure that it will not wait for other commands. I've already<br>
> implemented some kind of executor service (ConditionalExecutorService,<br>
> in ISPN-2635 and ISPN-2636 branches, Total Order stuff) that only put<br>
> the Runnable (more precisely a new interface called ConditionalRunnable)<br>
> in the thread pool when it is ready to be processed. Creative guys, it<br>
> may need a better name :)<br>
><br>
> The ConditionalRunnable has a new method (boolean isReady()) that should<br>
> return true when the runnable should not block.<br>
><br>
> Example how to apply this to ISPN-2808:<br>
><br>
> Most of the commands awaits for a particular topology ID and/or for lock<br>
> acquisition.<br>
><br>
><br>
> Well, the original problem description was about the<br>
> DistributionInterceptor forwarding the command from the primary owner to<br>
> the backup owners and waiting for a response from them :)<br>
> The forwarding done by StateTransferInterceptor is also synchronous and<br>
> can block.<br>
><br>
> It's true that you can't check how long a remote call will take<br>
> beforehand...<br>
><br>
> In this way, the isReady() implementation can be something<br>
> like:<br>
><br>
> isReady()<br>
> return commandTopologyId <= currentTopologyId && (for all keys; do if<br>
> !lock(key).tryLock(); return false; done)<br>
><br>
><br>
> Shouldn't you release the locks you managed to lock already if one of<br>
> the lock acquisitions failed?<br>
<br>
</div></div>you are right. I have to release the locks for the pessimist mode. In<br>
optimistic mode, the locks are only released with the rollback command.<br>
<div class="im">><br>
> Actually you may want to release the locks even if lock acquisition did<br>
> succeed... In non-tx mode, the locks are owned by the current thread, so<br>
> you can't lock a key on one thread and unlock it on another (though you<br>
> could skip the check completely in non-tx mode). And in transactional<br>
> mode, you could have a deadlock because 2 txs lock the same keys in a<br>
> different order.<br>
</div>Non-tx caches cannot use this optimization. I've seen that problem early<br>
today when I start debugging it.<br>
<div class="im">><br>
> Which leads me to a different point, how would you handle deadlocks?<br>
> With pessimistic mode, if tx1 holds lock k1 and wants to acquire k2, but<br>
> tx2 holds k2 and wants to acquire k1, will the LockCommands tx1:k2 and<br>
> tx2:k1 ever be scheduled? In general, can we make the time that a<br>
> Lock/PrepareCommand spends in the ConditionalExecutorService queue count<br>
> against lockAcquisitionTimeout?<br>
<br>
</div>If I got a DeadLockException, I will send it back immediately and<br>
release all the locks.<br>
<br></blockquote><div><br></div><div>Hmmm, the default LockManager doesn't throw DeadlockDetectedExceptions, you have to enable deadlockDetection explicitly in the configuration. I'm guessing that's for performance reasons...<br>
<br></div><div>It's true that you would need a new LockManager.tryLock method anyway, so you could implement deadlock detection in LockManagerImpl.tryLock. But probably the same performance considerations would apply.<br>
<br></div> <br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The lockAcquisitionTimeout is a problem that I haven't solved yet.<br>
<div><div class="h5"><br>
><br>
> With this, I believe we can keep the number of thread low and avoid the<br>
> thread deadlocks.<br>
><br>
> Now, I have two possible implementations:<br>
><br>
> 1) put a reference for StateTransferManager and/or LockManager in the<br>
> commands, and invoke the methods directly (a little dirty)<br>
><br>
> 2) added new method in the CommandInterceptor like: boolean<br>
> preProcess<command>(Command, InvocationContext). each interceptor will<br>
> check if the command will block on it (returning false) or not (invoke<br>
> the next interceptor). For example, the StateTransferInterceptor returns<br>
> immediately false if the commandToplogyId is higher than the<br>
> currentTopologyId and the *LockingIntercerptor will return false if it<br>
> cannot acquire some lock.<br>
><br>
> Any other suggestions? If I was not clear let me know.<br>
><br>
><br>
> TBH I would only check for the topology id before scheduling the<br>
> commands to the Infinispan thread pool, because checking the locks is<br>
> very complicated with all the configuration options that we have.<br>
><br>
> This is how I was imagining solving the lock problem: The OOB threads<br>
> would execute directly the commands that do not acquire locks<br>
> (CommitCommand, TxCompletionNotificationCommand) directly and submit the<br>
> others to the ISPN thread pool; if the command's topology id was higher<br>
> than the current topology id, the OOB thread would just stick it in a<br>
> queue. A separate thread would read only the commands with the current<br>
> topology id from the queue, and process them just like an OOB thread.<br>
><br>
> However... the CommitCommands may very well have a *lower* topology id<br>
> by the time they are executed, so the OOB/executor thread may well block<br>
> while waiting for the results of the forwarding RPC. Maybe we could work<br>
> around it by having StateTransferInterceptor submit a task with the<br>
> command forwarding only to the thread pool, but again it gets quite<br>
> complicated. So for the first phase I recommend handling only the<br>
> topology id problem.<br>
</div></div>I have a similar implementation, but in your example, the CommitCommand<br>
will be scheduler to the thread pool only if the command topology ID is<br>
lower or equals than the current topology ID (like all topology affected<br>
commands)<br>
<br>
Currently, I only have 3 tests failing in<br>
org.infinispan.replication.FlagsReplicationTest (2 assertions and 1<br>
timeout) that I'm checking now...<br>
<div class="HOEnZb"><div class="h5">><br></div></div></blockquote><div><br></div><div>I guess it's time to look at your code :)<br></div></div></div></div>