[infinispan-issues] [JBoss JIRA] (ISPN-2240) Per-key lock container leads to superfluous TimeoutExceptions on concurrent access to same key

[Robert Stupp (JIRA)]

    [ https://issues.jboss.org/browse/ISPN-2240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12715445#comment-12715445 ] 

Robert Stupp commented on ISPN-2240:
------------------------------------

Above recommendation works. Would be nice to get some feedback from the developers.

Temporary change of
{noformat}
org.infinispan.interceptors.locking.NonTransactionalLockingInterceptor#visitPutKeyValueCommand
{noformat}
to
{noformat}
   public Object visitPutKeyValueCommand(InvocationContext ctx, PutKeyValueCommand command) throws Throwable {
      assertNonTransactional(ctx);

      if (dm!=null && rpcManager!=null && ctx.isOriginLocal()) {
         Address primary = dm.getPrimaryLocation(command.getKey());
         Address local = rpcManager.getAddress();
         if (!local.equals(primary)) {
            SingleRpcCommand rpcCommand = commandsFactory.buildSingleRpcCommand(command);
            Map<Address, Response> response = rpcManager.invokeRemotely(Collections.singleton(primary), rpcCommand, ResponseMode.SYNCHRONOUS, 10000);
            Response resp = response.get(primary);
            if (resp==null) { // todo need to ignore L1 invalidation caused by current put
               entryFactory.wrapEntryForPut(ctx, command.getKey(), null, !command.isPutIfAbsent());
               MVCCEntry e = (MVCCEntry) ctx.lookupEntry(command.getKey());
               e.setValue(command.getValue());
               e.setLifespan(command.getLifespanMillis());
               e.setMaxIdle(command.getMaxIdleTimeMillis());
               return null;
            }
            if (resp instanceof SuccessfulResponse) {
               SuccessfulResponse sr = (SuccessfulResponse) resp;
               entryFactory.wrapEntryForPut(ctx, command.getKey(), null, !command.isPutIfAbsent());
               MVCCEntry e = (MVCCEntry) ctx.lookupEntry(command.getKey());
               e.setValue(command.getValue());
               e.setLifespan(command.getLifespanMillis());
               e.setMaxIdle(command.getMaxIdleTimeMillis());
               return sr.getResponseValue();
            }
            if (resp instanceof ExceptionResponse) {
               ExceptionResponse er = (ExceptionResponse)resp;
               throw er.getException();
            }
            throw new Error("hmmm ... "+resp);
         }
      }

      try {
         lockKey(ctx, command.getKey());
         return invokeNextInterceptor(ctx, command);
      } catch (Throwable te) {
         throw cleanLocksAndRethrow(ctx, te);
      }
      finally {
         lockManager.unlockAll(ctx);
      }
   }
{noformat}


                
> Per-key lock container leads to superfluous TimeoutExceptions on concurrent access to same key
> ----------------------------------------------------------------------------------------------
>
>                 Key: ISPN-2240
>                 URL: https://issues.jboss.org/browse/ISPN-2240
>             Project: Infinispan
>          Issue Type: Bug
>          Components: Locking and Concurrency
>    Affects Versions: 5.1.6.FINAL, 5.1.x
>            Reporter: Robert Stupp
>            Assignee: Mircea Markus
>         Attachments: ISPN-2240_fix_TimeoutExceptions.patch, somehow.zip
>
>
> Hi,
> I've encountered a lot of TimeoutExceptions just running a load test against an infinispan cluster.
> I tracked down the reason and found out, that the code in org.infinispan.util.concurrent.locks.containers.AbstractPerEntryLockContainer#releaseLock() causes these superfluous TimeoutExceptions.
> A small test case (which just prints out timeouts, too late timeouts and "paints" a lot of dots to the console - more dots/second on the console means better throughput ;-)
> In a short test I extended the class ReentrantPerEntryLockContainer and changed the implementation of releaseLock() as follows:
> {noformat}
>     public void releaseLock(Object lockOwner, Object key) {
>         ReentrantLock l = locks.get(key);
>         if (l != null) {
>             if (!l.isHeldByCurrentThread())
>                 throw new IllegalStateException("Lock for [" + key + "] not held by current thread " + Thread.currentThread());
>             while (l.isHeldByCurrentThread())
>                 unlock(l, lockOwner);
>             if (!l.hasQueuedThreads())
>                 locks.remove(key);
>         }
>         else
>             throw new IllegalStateException("No lock for [" + key + ']');
>     }
> {noformat}
> The main improvement is that locks are not removed from the concurrent map as long as other threads are waiting on that lock.
> If the lock is removed from the map while other threads are waiting for it, they may run into timeouts and force TimeoutExceptions to the client.
> The above methods "paints more dots per second" - means: it gives a better throughput for concurrent accesses to the same key.
> The re-implemented method should also fix some replication timeout exceptions.
> Please, please add this to 5.1.7, if possible.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira