replace synchronized(this) in MVStore with ReentrantLock

h2/src/main/org/h2/mvstore/MVStore.java

@@ -25,7 +25,7 @@ import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicLong;
-import java.util.concurrent.atomic.AtomicReference;
+import java.util.concurrent.locks.ReentrantLock;
 import org.h2.compress.CompressDeflate;
 import org.h2.compress.CompressLZF;
 import org.h2.compress.Compressor;
@@ -144,6 +144,14 @@ public class MVStore {
      */
     private static final int MARKED_FREE = 10_000_000;
 
+    /**
+     * Lock which governs access to major store operations: store(), close(), ...
+     * It should used in a non-reentrant fashion.
+     * It serves as a replacement for synchronized(this), except it allows for
+     * non-blocking lock attempts.
+     */
+    private final ReentrantLock storeLock = new ReentrantLock();
+
     /**
      * The background thread, if any.
      */
@@ -195,8 +203,7 @@ public class MVStore {
     private final Map<Integer, Chunk> freedPageSpace = new HashMap<>();
 
     /**
-     * The metadata map. Write access to this map needs to be synchronized on
-     * the store.
+     * The metadata map. Write access to this map needs to be done under storeLock.
      */
     private final MVMap<String, String> meta;
 
@@ -274,12 +281,6 @@ public class MVStore {
      */
     private volatile long currentStoreVersion = -1;
 
-    /**
-     * Holds reference to a thread performing store operation (if any)
-     * or null if there is none is in progress.
-     */
-    private final AtomicReference<Thread> currentStoreThread = new AtomicReference<>();
-
     private volatile boolean metaChanged;
 
     /**
@@ -289,7 +290,9 @@ public class MVStore {
 
     private final int autoCompactFillRate;
     private long autoCompactLastFileOpCount;
-
+    /**
+     * Simple lock to ensure that no more than one compaction runs at any given time
+     */
     private final Object compactSync = new Object();
 
     private volatile IllegalStateException panicException;
@@ -474,8 +477,9 @@ public class MVStore {
      * @param builder the map builder
      * @return the map
      */
-    public synchronized <M extends MVMap<K, V>, K, V> M openMap(
-            String name, MVMap.MapBuilder<M, K, V> builder) {
+    public <M extends MVMap<K, V>, K, V> M openMap(String name, MVMap.MapBuilder<M, K, V> builder) {
+        storeLock.lock();
+        try {
             int id = getMapId(name);
             M map;
             if (id >= 0) {
@@ -493,16 +497,20 @@ public class MVStore {
                 map.setRootPos(0, lastStoredVersion);
                 markMetaChanged();
                 @SuppressWarnings("unchecked")
-            M existingMap = (M)maps.putIfAbsent(id, map);
-            if(existingMap != null) {
+                M existingMap = (M) maps.putIfAbsent(id, map);
+                if (existingMap != null) {
                     map = existingMap;
                 }
             }
             return map;
+        } finally {
+            storeLock.unlock();
+        }
     }
 
-    public synchronized <M extends MVMap<K, V>, K, V> M openMap(int id,
-                                        MVMap.MapBuilder<M, K, V> builder) {
+    public <M extends MVMap<K, V>, K, V> M openMap(int id, MVMap.MapBuilder<M, K, V> builder) {
+        storeLock.lock();
+        try {
             @SuppressWarnings("unchecked")
             M map = (M) getMap(id);
             if (map == null) {
@@ -519,6 +527,9 @@ public class MVStore {
                 }
             }
             return map;
+        } finally {
+            storeLock.unlock();
+        }
     }
 
     public <K, V> MVMap<K,V> getMap(int id) {
@@ -954,12 +965,10 @@ public class MVStore {
         if (closed) {
             return;
         }
-        // can not synchronize on this yet, because
-        // the thread also synchronized on this, which
-        // could result in a deadlock
         stopBackgroundThread();
         closed = true;
-        synchronized (this) {
+        storeLock.lock();
+        try {
             if (fileStore != null && shrinkIfPossible) {
                 shrinkFileIfPossible(0);
             }
@@ -979,6 +988,8 @@ public class MVStore {
             if (fileStore != null && !fileStoreIsProvided) {
                 fileStore.close();
             }
+        } finally {
+            storeLock.unlock();
         }
     }
 
@@ -1043,11 +1054,15 @@ public class MVStore {
      * @return the new version (incremented if there were changes)
      */
     public long tryCommit() {
-        // unlike synchronization, this will also prevent re-entrance,
-        // which may be possible, if the meta map have changed
-        if (currentStoreThread.compareAndSet(null, Thread.currentThread())) {
-            synchronized (this) {
+        // we need to prevent re-entrance, which may be possible,
+        // because meta map is modified within storeNow() and that
+        // causes beforeWrite() call with possibility of going back here
+        if ((!storeLock.isHeldByCurrentThread() || currentStoreVersion < 0) &&
+                storeLock.tryLock()) {
+            try {
                 store();
+            } finally {
+                storeLock.unlock();
             }
         }
         return currentVersion;
@@ -1069,15 +1084,16 @@ public class MVStore {
      *
      * @return the new version (incremented if there were changes)
      */
-    public synchronized long commit() {
-        Thread currentThread = Thread.currentThread();
-        Thread storeThread = currentStoreThread.get();
-        if (currentThread != storeThread) { // to avoid re-entrance
-            currentStoreThread.set(currentThread);
+    public long commit() {
+        // we need to prevent re-entrance, which may be possible,
+        // because meta map is modified within storeNow() and that
+        // causes beforeWrite() call with possibility of going back here
+        if(!storeLock.isHeldByCurrentThread() || currentStoreVersion < 0) {
+            storeLock.lock();
             try {
                 store();
             } finally {
-                currentStoreThread.set(storeThread);
+                storeLock.unlock();
             }
         }
         return currentVersion;
@@ -1111,12 +1127,11 @@ public class MVStore {
             // in any case reset the current store version,
             // to allow closing the store
             currentStoreVersion = -1;
-            currentStoreThread.set(null);
         }
     }
 
     private void storeNow() {
-        assert Thread.holdsLock(this);
+        assert storeLock.isHeldByCurrentThread();
         long time = getTimeSinceCreation();
         freeUnusedIfNeeded(time);
         int currentUnsavedPageCount = unsavedMemory;
@@ -1322,7 +1337,8 @@ public class MVStore {
         }
     }
 
-    private synchronized void freeUnusedChunks() {
+    private void freeUnusedChunks() {
+        assert storeLock.isHeldByCurrentThread();
         if (lastChunk != null && reuseSpace) {
             Set<Integer> referenced = collectReferencedChunks();
             long time = getTimeSinceCreation();
@@ -1693,7 +1709,9 @@ public class MVStore {
      *
      * @return if anything was written
      */
-    public synchronized boolean compactRewriteFully() {
+    public boolean compactRewriteFully() {
+        storeLock.lock();
+        try {
             checkOpen();
             if (lastChunk == null) {
                 // nothing to do
@@ -1718,6 +1736,10 @@ public class MVStore {
             }
             commit();
             return true;
+        } finally {
+            storeLock.unlock();
+        }
+
     }
 
     /**
@@ -1737,7 +1759,9 @@ public class MVStore {
      *            than this
      * @param moveSize the number of bytes to move
      */
-    public synchronized void compactMoveChunks(int targetFillRate, long moveSize) {
+    public void compactMoveChunks(int targetFillRate, long moveSize) {
+        storeLock.lock();
+        try {
             checkOpen();
             if (lastChunk != null && reuseSpace) {
                 int oldRetentionTime = retentionTime;
@@ -1755,6 +1779,9 @@ public class MVStore {
                     retentionTime = oldRetentionTime;
                 }
             }
+        } finally {
+            storeLock.unlock();
+        }
     }
 
     private ArrayList<Chunk> findChunksToMove(long startBlock, long moveSize) {
@@ -1896,16 +1923,24 @@ public class MVStore {
         synchronized (compactSync) {
             checkOpen();
             ArrayList<Chunk> old;
-            synchronized (this) {
+            // We can't wait fo lock here, because if called from the background thread,
+            // it might go into deadlock with concurrent database closure
+            // and attempt to stop this thread.
+            if (storeLock.tryLock()) {
+                try {
                     old = findOldChunks(targetFillRate, write);
-            }
                     if (old == null || old.isEmpty()) {
                         return false;
                     }
                     compactRewrite(old);
                     return true;
+                } finally {
+                    storeLock.unlock();
+                }
             }
         }
+        return false;
+    }
 
     /**
      * Get the current fill rate (percentage of used space in the file). Unlike
@@ -2336,10 +2371,15 @@ public class MVStore {
      *
      * @param version the new store version
      */
-    public synchronized void setStoreVersion(int version) {
+    public void setStoreVersion(int version) {
+        storeLock.lock();
+        try {
             checkOpen();
             markMetaChanged();
             meta.put("setting.storeVersion", Integer.toHexString(version));
+        } finally {
+            storeLock.unlock();
+        }
     }
 
     /**
@@ -2358,7 +2398,9 @@ public class MVStore {
      *
      * @param version the version to revert to
      */
-    public synchronized void rollbackTo(long version) {
+    public void rollbackTo(long version) {
+        storeLock.lock();
+        try {
             checkOpen();
             if (version == 0) {
                 // special case: remove all data
@@ -2457,6 +2499,9 @@ public class MVStore {
             if (lastStoredVersion == INITIAL_VERSION) {
                 lastStoredVersion = currentVersion - 1;
             }
+        } finally {
+            storeLock.unlock();
+        }
     }
 
     private static long getRootPos(MVMap<String, String> map, int mapId) {
@@ -2544,7 +2589,9 @@ public class MVStore {
         removeMap(map, true);
     }
 
-    public synchronized void removeMap(MVMap<?, ?> map, boolean delayed) {
+    public void removeMap(MVMap<?, ?> map, boolean delayed) {
+        storeLock.lock();
+        try {
             checkOpen();
             DataUtils.checkArgument(map != meta,
                     "Removing the meta map is not allowed");
@@ -2556,6 +2603,9 @@ public class MVStore {
             int id = map.getId();
             String name = getMapName(id);
             removeMap(name, id, delayed);
+        } finally {
+            storeLock.unlock();
+        }
     }
 
     private void removeMap(String name, int id, boolean delayed) {
@@ -2681,11 +2731,7 @@ public class MVStore {
         synchronized (t.sync) {
             t.sync.notifyAll();
         }
-        if (Thread.holdsLock(this)) {
-            // called from storeNow: can not join,
-            // because that could result in a deadlock
-            return;
-        }
+
         try {
             t.join();
         } catch (Exception e) {
@@ -2855,11 +2901,11 @@ public class MVStore {
     public void deregisterVersionUsage(TxCounter txCounter) {
         if(txCounter != null) {
             if(txCounter.counter.decrementAndGet() <= 0) {
-                if (currentStoreThread.compareAndSet(null, Thread.currentThread())) {
+                if (!storeLock.isHeldByCurrentThread() && storeLock.tryLock()) {
                     try {
                         dropUnusedVersions();
                     } finally {
-                        currentStoreThread.set(null);
+                        storeLock.unlock();
                     }
                 }
             }