From ecee392a39349a6523d1ba8944321471d3cc7811 Mon Sep 17 00:00:00 2001
From: Stepan Usatiuk <stepan@usatiuk.com>
Date: Sat, 15 Feb 2025 22:02:31 +0100
Subject: [PATCH] fix deadlock and possible inconsistency in transactions

---
 .../usatiuk/dhfs/objects/JObjectManager.java  | 44 ++++++++++---------
 .../transaction/TransactionFactoryImpl.java   |  5 +++
 .../transaction/TransactionPrivate.java       |  2 +
 3 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/dhfs-parent/objects/src/main/java/com/usatiuk/dhfs/objects/JObjectManager.java b/dhfs-parent/objects/src/main/java/com/usatiuk/dhfs/objects/JObjectManager.java
index d72cc71d..eb4c466d 100644
--- a/dhfs-parent/objects/src/main/java/com/usatiuk/dhfs/objects/JObjectManager.java
+++ b/dhfs-parent/objects/src/main/java/com/usatiuk/dhfs/objects/JObjectManager.java
@@ -18,6 +18,7 @@ import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.function.Consumer;
 import java.util.function.Function;
+import java.util.stream.Stream;
 
 // Manages all access to com.usatiuk.dhfs.objects.JData objects.
 // In particular, it serves as a source of truth for what is committed to the backing storage.
@@ -130,27 +131,22 @@ public class JObjectManager {
                     });
                 };
 
-        Function<JObjectKey, JData> getCurrent =
-                key -> switch (current.get(key)) {
-                    case TxRecord.TxObjectRecordWrite<?> write -> write.data();
-                    case TxRecord.TxObjectRecordDeleted deleted -> null;
-                    case null -> {
-                        var dep = dependenciesLocked.get(key);
-                        if (dep == null) {
-                            throw new TxCommitException("No dependency for " + key);
-                        }
-                        yield dep.data.map(JDataVersionedWrapper::data).orElse(null);
-                    }
-                    default -> {
-                        throw new TxCommitException("Unexpected value: " + current.get(key));
-                    }
-                };
-
         // For existing objects:
         // Check that their version is not higher than the version of transaction being committed
         // TODO: check deletions, inserts
         try {
             try {
+                Function<JObjectKey, JData> getCurrent =
+                        key -> switch (current.get(key)) {
+                            case TxRecord.TxObjectRecordWrite<?> write -> write.data();
+                            case TxRecord.TxObjectRecordDeleted deleted -> null;
+                            case null ->
+                                    tx.readSource().get(JData.class, key).data().map(JDataVersionedWrapper::data).orElse(null);
+                            default -> {
+                                throw new TxCommitException("Unexpected value: " + current.get(key));
+                            }
+                        };
+
                 boolean somethingChanged;
                 do {
                     somethingChanged = false;
@@ -160,10 +156,6 @@ public class JObjectManager {
                             currentIteration.put(n.key(), n);
                         Log.trace("Commit iteration with " + currentIteration.size() + " records for hook " + hook.getClass());
 
-                        currentIteration.keySet().stream()
-                                .sorted(Comparator.comparing(JObjectKey::toString))
-                                .forEach(addDependency);
-
                         for (var entry : currentIteration.entrySet()) {
                             // FIXME: Kinda hack?
                             if (entry.getKey().equals(JDataDummy.TX_ID_OBJ_NAME)) {
@@ -191,8 +183,12 @@ public class JObjectManager {
                 } while (somethingChanged);
             } finally {
                 reads = tx.reads();
+
+                Stream.concat(reads.keySet().stream(), current.keySet().stream())
+                        .sorted(Comparator.comparing(JObjectKey::toString))
+                        .forEach(addDependency);
+
                 for (var read : reads.entrySet()) {
-                    addDependency.accept(read.getKey());
                     if (read.getValue() instanceof TransactionObjectLocked<?> locked) {
                         toUnlock.add(locked.lock);
                     }
@@ -207,7 +203,13 @@ public class JObjectManager {
                     continue;
                 }
 
+                if (dep.data().orElse(null) != read.getValue().data().orElse(null)) {
+                    Log.trace("Checking dependency " + read.getKey() + " - changed already");
+                    throw new TxCommitException("Serialization hazard: " + dep.data().get().version() + " vs " + tx.getId());
+                }
+
                 if (dep.data().get().version() >= tx.getId()) {
+                    assert false;
                     Log.trace("Checking dependency " + read.getKey() + " - newer than");
                     throw new TxCommitException("Serialization hazard: " + dep.data().get().version() + " vs " + tx.getId());
                 }
diff --git a/dhfs-parent/objects/src/main/java/com/usatiuk/dhfs/objects/transaction/TransactionFactoryImpl.java b/dhfs-parent/objects/src/main/java/com/usatiuk/dhfs/objects/transaction/TransactionFactoryImpl.java
index 5cef472d..f7357822 100644
--- a/dhfs-parent/objects/src/main/java/com/usatiuk/dhfs/objects/transaction/TransactionFactoryImpl.java
+++ b/dhfs-parent/objects/src/main/java/com/usatiuk/dhfs/objects/transaction/TransactionFactoryImpl.java
@@ -97,6 +97,11 @@ public class TransactionFactoryImpl implements TransactionFactory {
         public Map<JObjectKey, TransactionObject<?>> reads() {
             return _source.getRead();
         }
+
+        @Override
+        public ReadTrackingObjectSource readSource() {
+            return _source;
+        }
     }
 
 }
diff --git a/dhfs-parent/objects/src/main/java/com/usatiuk/dhfs/objects/transaction/TransactionPrivate.java b/dhfs-parent/objects/src/main/java/com/usatiuk/dhfs/objects/transaction/TransactionPrivate.java
index 4229e939..e7ca7d05 100644
--- a/dhfs-parent/objects/src/main/java/com/usatiuk/dhfs/objects/transaction/TransactionPrivate.java
+++ b/dhfs-parent/objects/src/main/java/com/usatiuk/dhfs/objects/transaction/TransactionPrivate.java
@@ -10,4 +10,6 @@ public interface TransactionPrivate extends Transaction {
     Collection<TxRecord.TxObjectRecord<?>> drainNewWrites();
 
     Map<JObjectKey, TransactionObject<?>> reads();
+
+    ReadTrackingObjectSource readSource();
 }