ISPN-8533 Deadlock in pessimistic transaction #6075

pruivo · 2018-06-19T13:12:25Z

https://issues.jboss.org/browse/ISPN-8533

pruivo · 2018-06-19T13:13:14Z

run performance tests please

ghost · 2018-06-19T13:16:15Z

Performance tests didn't finish successfully. @diegolovison, can you review it?

Additional info:
Commit: ea8dd12
Build number: #254
Comment body: run performance tests please

diegolovison · 2018-06-19T13:24:37Z

run performance tests please

ghost · 2018-06-19T13:31:03Z

Performance tests didn't finish successfully. @diegolovison, can you review it?

Additional info:
Commit: ea8dd12
Build number: #255
Comment body: run performance tests please

diegolovison · 2018-06-19T13:36:05Z

@pruivo, @mgencur is running some tests in our perf lab.
We will need to wait a little bit

pruivo · 2018-06-19T13:44:52Z

@diegolovison ok! thanks!

diegolovison · 2018-06-19T15:51:39Z

run performance tests please

ghost · 2018-06-19T19:47:24Z

Performance tests run successfully. Link to the results here.

Additional info:
Commit: ea8dd12
Build number: #256
Comment body: run performance tests please

tristantarrant · 2018-06-20T09:13:36Z

Perf looks good.

galderz · 2018-06-22T08:43:27Z

Some connection here too (like in #6045), restarted build

gustavocoding · 2018-06-22T08:45:32Z

Gotta love the CI stability...

danberindei

Sorry Pedro, I forgot about this yesterday.

I'm still not sure what this change does TBH, I see you've split backupLockReleased into multiple CFs, but it's not clear how that fixes the problem :)

danberindei · 2018-06-22T08:52:27Z

...src/main/java/org/infinispan/interceptors/distribution/VersionedDistributionInterceptor.java

      // TODO: should we check the write skew configuration here?
      // TODO: version seen or looked up remote version?
-      if (lastVersion != null && lastVersion.compareTo(version) != InequalVersionComparisonResult.EQUAL) {
-         throw log.writeSkewOnRead(key, key, lastVersion, version);
-      }


What does this do? The JIRA only talks about pessimistic transactions

lastVersion is always null since getLookedUpRemoteVersion() is deprecated since 9.0 and always return null.

danberindei · 2018-06-22T08:52:45Z

core/src/main/java/org/infinispan/interceptors/locking/PessimisticLockingInterceptor.java

@@ -88,6 +88,7 @@ private KeyAwareLockPromise acquireLocalLock(InvocationContext ctx, DataCommand
      final TxInvocationContext txContext = (TxInvocationContext) ctx;
      Object key = command.getKey();
      txContext.addAffectedKey(key);
+      ((TxInvocationContext) ctx).getCacheTransaction().cleanupBackupLocksForKey(key);


danberindei · 2018-06-22T08:55:52Z

core/src/main/java/org/infinispan/remoting/inboundhandler/action/PendingTxAction.java

         //clear the backup locks
         context.getCacheTransaction().cleanupBackupLocks();
         keysToLock.removeAll(context.getLockedKeys());
      }
+      if (command instanceof LockControlCommand) {
+         //the lock command is only issue if the transaction doesn't have the lock for the keys.


issued?

And I'd like to see a comment above these 2 blocks explaining why we need to remove backup locks first

danberindei · 2018-06-22T08:57:42Z

core/src/main/java/org/infinispan/statetransfer/StateProviderImpl.java

-         }
+         //avoids the warning about synchronizing in a local variable.
+         //and allows us to change the CacheTransaction internals without having to worry about it
+         tx.collectBackupLockKeysForSegments(segments, keyPartitioner, filteredLockedKeys);


Here instead I don't think a comment isn't necessary, it's just common sense. However, I would have liked the extracted method to handle both regular locks and backup locks, and I would have preferred a more generic method like forEachLock.

danberindei · 2018-06-22T09:02:26Z

core/src/main/java/org/infinispan/transaction/impl/AbstractCacheTransaction.java

      // we need a synchronized collection to be able to get a valid snapshot from another thread during state transfer
-      final Set<Object> keys = backupKeyLocks.updateAndGet((value) -> value == null ? Collections.synchronizedSet(new HashSet<>(INITIAL_LOCK_CAPACITY)) : value);
-      keys.add(key);
+      if (backupKeyLocks == null) {


The comment above is no longer true

danberindei · 2018-06-22T09:40:18Z

core/src/main/java/org/infinispan/transaction/xa/CacheTransaction.java

+   /**
+    * testing purpose only!
+    */
+   @Deprecated


You should deprecate the implementation as well, and replace the javadoc with something like @deprecated Since 9.3, please use ...

danberindei · 2018-06-22T09:42:50Z

core/src/test/java/org/infinispan/tx/PessimisticDeadlockTest.java

+public class PessimisticDeadlockTest extends MultipleCacheManagersTest {
+
+   public void testDeadlock() throws Exception {
+      assertEquals(4, managers().length);


Hmmm, maybe you should move createCacheManagers() so it's obvious without having to assert :)

danberindei · 2018-06-22T09:53:08Z

core/src/test/java/org/infinispan/tx/PessimisticDeadlockTest.java

+      ConfigurationBuilder builder = getDefaultClusteredCacheConfig(CacheMode.DIST_SYNC, true);
+      builder.transaction().lockingMode(LockingMode.PESSIMISTIC);
+      builder.clustering().hash().consistentHashFactory(new ControlledConsistentHashFactory.Default(1, 2))
+            .numSegments(1)


Don't really need MagicKey if you only have 1 segment. But this got me thinking, should this test have more scenarios to test merging locks into an existing transaction (if the originator was backup/non-owner and becomes primary owner).

danberindei · 2018-06-22T09:57:55Z

core/src/test/java/org/infinispan/tx/PessimisticDeadlockTest.java

+         Iterator<Address> iterator = writeOwners.iterator();
+         assertEquals(address(1), iterator.next());
+         assertEquals(address(2), iterator.next());
+         assertFalse(writeOwners.contains(address(0)));


How about assertEquals(Arrays.asList(address(1), address(2)), writeOwners)?

danberindei · 2018-06-22T10:07:05Z

core/src/main/java/org/infinispan/transaction/impl/AbstractCacheTransaction.java

@@ -59,7 +61,9 @@
   private final AtomicReference<Set<Object>> lockedKeys = new AtomicReference<>();

   /** Holds all the locks for which the local node is a secondary data owner. */
-   private final AtomicReference<Set<Object>> backupKeyLocks = new AtomicReference<>();
+   @GuardedBy("this")
+   private Map<Object, CompletableFuture<Void>> backupKeyLocks;


Could you modify the comment to say what these CompletableFutures actually represent?

danberindei · 2018-06-22T10:42:59Z

core/src/main/java/org/infinispan/transaction/impl/LocalTransaction.java

-      if (entry instanceof RepeatableReadEntry) {
-         return ((RepeatableReadEntry) entry).isRead();
-      } else {
-         return false;


When possible, I prefer keeping deprecated methods working as before instead of keeping only a skeleton for compilation reasons.

sorry I didn't understand what you mean :(

I mean if the method still works, I don't see any reason to replace it with an empty method.

danberindei · 2018-06-22T12:07:14Z

core/src/main/java/org/infinispan/remoting/inboundhandler/action/PendingTxAction.java

         //clear the backup locks
         context.getCacheTransaction().cleanupBackupLocks();
         keysToLock.removeAll(context.getLockedKeys());
      }
+      if (command instanceof LockControlCommand) {
+         //the lock command is only issued if the transaction doesn't have the lock for the keys.


I still don't know why we have to remove the backup locks here...

danberindei · 2018-06-22T12:08:13Z

core/src/main/java/org/infinispan/statetransfer/StateProviderImpl.java

-               }
+         //avoids the warning about synchronizing in a local variable.
+         //and allows us to change the CacheTransaction internals without having to worry about it
+         tx.forEachBackupLock(key -> {


Why not forEachLock?

because it isn't related to this PR (?) Do you want to replace all the getLockedKeys() with a forEachLock() ?

Ok, I'll wait for https://issues.jboss.org/browse/ISPN-3927 then :)

danberindei · 2018-06-22T12:09:34Z

core/src/main/java/org/infinispan/transaction/impl/AbstractCacheTransaction.java

+    * <p>
+    * A {@link CompletableFuture} is created for each key and it is completed when the backup lock is release for that
+    * key. A transaction, before acquiring the locks, must wait for all the backup locks (i.e. the {@link
+    * CompletableFuture}) for all transaction created in the previous topology.


Nitpicking: is released, all transactions

* Minor deprecation cleanups

danberindei · 2018-06-22T15:00:28Z

Integrated, thanks Pedro!

pruivo added the Performance ACK required! label Jun 19, 2018

tristantarrant added this to the 9.3.0.Final milestone Jun 21, 2018

tristantarrant removed the Performance ACK required! label Jun 21, 2018

danberindei suggested changes Jun 22, 2018

View reviewed changes

danberindei added the Changes Required label Jun 22, 2018

danberindei reviewed Jun 22, 2018

View reviewed changes

pruivo force-pushed the t_8533 branch from ea8dd12 to d652489 Compare June 22, 2018 11:29

danberindei reviewed Jun 22, 2018

View reviewed changes

pruivo added 2 commits June 22, 2018 14:19

ISPN-8533 Deadlock in pessimistic transaction

2ebe6fd

ISPN-8533 Deadlock in pessimistic transaction

36c4340

* Minor deprecation cleanups

pruivo force-pushed the t_8533 branch from d652489 to 36c4340 Compare June 22, 2018 13:23

danberindei approved these changes Jun 22, 2018

View reviewed changes

danberindei removed the Changes Required label Jun 22, 2018

danberindei merged commit 682e6f0 into infinispan:master Jun 22, 2018

pruivo deleted the t_8533 branch July 5, 2022 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISPN-8533 Deadlock in pessimistic transaction #6075

ISPN-8533 Deadlock in pessimistic transaction #6075

pruivo commented Jun 19, 2018

pruivo commented Jun 19, 2018

ghost commented Jun 19, 2018

diegolovison commented Jun 19, 2018

ghost commented Jun 19, 2018

diegolovison commented Jun 19, 2018 •

edited

pruivo commented Jun 19, 2018

diegolovison commented Jun 19, 2018

ghost commented Jun 19, 2018

tristantarrant commented Jun 20, 2018

galderz commented Jun 22, 2018

gustavocoding commented Jun 22, 2018

danberindei left a comment

danberindei Jun 22, 2018

pruivo Jun 22, 2018

danberindei Jun 22, 2018

danberindei Jun 22, 2018

danberindei Jun 22, 2018

danberindei Jun 22, 2018

danberindei Jun 22, 2018

danberindei Jun 22, 2018

danberindei Jun 22, 2018

danberindei Jun 22, 2018

danberindei Jun 22, 2018

danberindei Jun 22, 2018

pruivo Jun 22, 2018

danberindei Jun 22, 2018

danberindei Jun 22, 2018

danberindei Jun 22, 2018

pruivo Jun 22, 2018

danberindei Jun 22, 2018

danberindei Jun 22, 2018

danberindei commented Jun 22, 2018

ISPN-8533 Deadlock in pessimistic transaction #6075

ISPN-8533 Deadlock in pessimistic transaction #6075

Conversation

pruivo commented Jun 19, 2018

pruivo commented Jun 19, 2018

ghost commented Jun 19, 2018

diegolovison commented Jun 19, 2018

ghost commented Jun 19, 2018

diegolovison commented Jun 19, 2018 • edited

pruivo commented Jun 19, 2018

diegolovison commented Jun 19, 2018

ghost commented Jun 19, 2018

tristantarrant commented Jun 20, 2018

galderz commented Jun 22, 2018

gustavocoding commented Jun 22, 2018

danberindei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danberindei commented Jun 22, 2018

diegolovison commented Jun 19, 2018 •

edited