HDDS-13978. OMLockDetails should not be used as the object returns a ThreadLocal Object (#9346)

swamirishi · web-flow · commit c8bcf7f7d64e · 2025-11-24T18:04:40.000-05:00
diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/MultiSnapshotLocks.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/MultiSnapshotLocks.java
@@ -67,8 +67,10 @@ public synchronized OMLockDetails acquireLock(Collection<UUID> ids) throws OMExc
         lock.acquireReadLocks(resource, keys);
     if (omLockDetails.isLockAcquired()) {
       objectLocks.addAll(keys);
+      this.lockDetails = OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED;
+    } else {
+      this.lockDetails = OMLockDetails.EMPTY_DETAILS_LOCK_NOT_ACQUIRED;
     }
-    this.lockDetails = omLockDetails;
     return omLockDetails;
   }
 
@@ -78,6 +80,8 @@ public synchronized void releaseLock() {
     } else {
       lockDetails = lock.releaseReadLocks(resource, this.objectLocks);
     }
+    this.lockDetails = lockDetails.isLockAcquired() ? OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED :
+        OMLockDetails.EMPTY_DETAILS_LOCK_NOT_ACQUIRED;
     this.objectLocks.clear();
   }
 
diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java
@@ -18,6 +18,8 @@
 package org.apache.hadoop.ozone.om.snapshot;
 
 import static org.apache.hadoop.ozone.om.lock.FlatResource.SNAPSHOT_DB_LOCK;
+import static org.apache.hadoop.ozone.om.lock.OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED;
+import static org.apache.hadoop.ozone.om.lock.OMLockDetails.EMPTY_DETAILS_LOCK_NOT_ACQUIRED;
 import static org.apache.ozone.rocksdiff.RocksDBCheckpointDiffer.COLUMN_FAMILIES_TO_TRACK_IN_DAG;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -304,13 +306,20 @@ public UncheckedAutoCloseableSupplier<OMLockDetails> lock(UUID snapshotId) {
         () -> cleanup(snapshotId));
   }
 
+  private OMLockDetails getEmptyOmLockDetails(OMLockDetails lockDetails) {
+    return lockDetails.isLockAcquired() ? EMPTY_DETAILS_LOCK_ACQUIRED : EMPTY_DETAILS_LOCK_NOT_ACQUIRED;
+  }
+
   private UncheckedAutoCloseableSupplier<OMLockDetails> lock(Supplier<OMLockDetails> lockFunction,
       Supplier<OMLockDetails> unlockFunction, Supplier<Void> cleanupFunction) {
-    AtomicReference<OMLockDetails> lockDetails = new AtomicReference<>(lockFunction.get());
+    Supplier<OMLockDetails> emptyLockFunction = () -> getEmptyOmLockDetails(lockFunction.get());
+    Supplier<OMLockDetails> emptyUnlockFunction = () -> getEmptyOmLockDetails(unlockFunction.get());
+
+    AtomicReference<OMLockDetails> lockDetails = new AtomicReference<>(emptyLockFunction.get());
     if (lockDetails.get().isLockAcquired()) {
       cleanupFunction.get();
       if (!dbMap.isEmpty()) {
-        lockDetails.set(unlockFunction.get());
+        lockDetails.set(emptyUnlockFunction.get());
       }
     }
 
@@ -320,7 +329,7 @@ private UncheckedAutoCloseableSupplier<OMLockDetails> lock(Supplier<OMLockDetail
       public void close() {
         lockDetails.updateAndGet((prevLock) -> {
           if (prevLock != null && prevLock.isLockAcquired()) {
-            return unlockFunction.get();
+            return emptyUnlockFunction.get();
           }
           return prevLock;
         });
diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestMultiSnapshotLocks.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestMultiSnapshotLocks.java
@@ -17,6 +17,7 @@
 
 package org.apache.hadoop.ozone.om.snapshot;
 
+import static org.apache.hadoop.ozone.om.lock.FlatResource.SNAPSHOT_GC_LOCK;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -31,9 +32,11 @@
 import static org.mockito.Mockito.when;
 
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import java.util.UUID;
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
 import org.apache.hadoop.ozone.om.exceptions.OMException;
 import org.apache.hadoop.ozone.om.lock.IOzoneManagerLock;
 import org.apache.hadoop.ozone.om.lock.OMLockDetails;
@@ -66,6 +69,21 @@ void setUp() {
     multiSnapshotLocks = new MultiSnapshotLocks(mockLock, mockResource, true);
   }
 
+  @Test
+  public void testMultiSnapshotLocksWithMultipleResourceLocksMultipleTimes() throws OMException {
+    OzoneManagerLock omLock = new OzoneManagerLock(new OzoneConfiguration());
+    MultiSnapshotLocks multiSnapshotLocks1 = new MultiSnapshotLocks(omLock, SNAPSHOT_GC_LOCK, true);
+    MultiSnapshotLocks multiSnapshotLocks2 = new MultiSnapshotLocks(omLock, SNAPSHOT_GC_LOCK, true);
+    Collection<UUID> uuid1 = Collections.singleton(UUID.randomUUID());
+    Collection<UUID> uuid2 = Collections.singleton(UUID.randomUUID());
+    for (int i = 0; i < 10; i++) {
+      assertTrue(multiSnapshotLocks1.acquireLock(uuid1).isLockAcquired());
+      assertTrue(multiSnapshotLocks2.acquireLock(uuid2).isLockAcquired());
+      multiSnapshotLocks1.releaseLock();
+      multiSnapshotLocks2.releaseLock();
+    }
+  }
+
   @Test
   void testAcquireLockSuccess() throws Exception {
     List<UUID> objects = Arrays.asList(obj1, obj2);
@@ -107,7 +125,8 @@ void testReleaseLock() throws Exception {
     when(mockLock.acquireWriteLocks(eq(mockResource), anyCollection())).thenReturn(mockLockDetails);
     multiSnapshotLocks.acquireLock(objects);
     assertFalse(multiSnapshotLocks.getObjectLocks().isEmpty());
-
+    when(mockLockDetails.isLockAcquired()).thenReturn(false);
+    when(mockLock.releaseWriteLocks(eq(mockResource), anyCollection())).thenReturn(mockLockDetails);
     // Now release locks
     multiSnapshotLocks.releaseLock();
 
diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotCache.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotCache.java
@@ -18,12 +18,14 @@
 package org.apache.hadoop.ozone.om.snapshot;
 
 import static org.apache.hadoop.ozone.om.lock.FlatResource.SNAPSHOT_DB_LOCK;
+import static org.apache.hadoop.ozone.om.lock.OzoneManagerLock.LeveledResource.VOLUME_LOCK;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertInstanceOf;
 import static org.junit.jupiter.api.Assertions.assertNotEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
@@ -43,6 +45,7 @@
 import java.util.concurrent.Semaphore;
 import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicBoolean;
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
 import org.apache.hadoop.hdds.utils.db.Table;
 import org.apache.hadoop.ozone.om.OMMetadataManager;
 import org.apache.hadoop.ozone.om.OMMetrics;
@@ -51,6 +54,7 @@
 import org.apache.hadoop.ozone.om.lock.IOzoneManagerLock;
 import org.apache.hadoop.ozone.om.lock.OMLockDetails;
 import org.apache.hadoop.ozone.om.lock.OmReadOnlyLock;
+import org.apache.hadoop.ozone.om.lock.OzoneManagerLock;
 import org.apache.ozone.test.GenericTestUtils;
 import org.apache.ratis.util.function.UncheckedAutoCloseableSupplier;
 import org.junit.jupiter.api.AfterEach;
@@ -189,6 +193,17 @@ public void testLockHoldsWriteLock(int numberOfLocks) {
     verify(lock, times(numberOfLocks)).acquireResourceWriteLock(eq(SNAPSHOT_DB_LOCK));
   }
 
+  @Test
+  public void testLockSupplierReturnsLockWithAnotherLockReleased() {
+    IOzoneManagerLock ozoneManagerLock = new OzoneManagerLock(new OzoneConfiguration());
+    snapshotCache = new SnapshotCache(cacheLoader, CACHE_SIZE_LIMIT, omMetrics, 50, true, ozoneManagerLock);
+    try (UncheckedAutoCloseableSupplier<OMLockDetails> lockDetails = snapshotCache.lock()) {
+      ozoneManagerLock.acquireWriteLock(VOLUME_LOCK, "vol1");
+      ozoneManagerLock.releaseWriteLock(VOLUME_LOCK, "vol1");
+      assertTrue(lockDetails.get().isLockAcquired());
+    }
+  }
+
   @ParameterizedTest
   @ValueSource(ints = {0, 1, 5, 10})
   @DisplayName("Tests lock(snapshotId) holds a write lock")

Original file line number	Diff line number	Diff line change
`@@ -67,8 +67,10 @@ public synchronized OMLockDetails acquireLock(Collection<UUID> ids) throws OMExc`
`67`	`67`	`lock.acquireReadLocks(resource, keys);`
`68`	`68`	`if (omLockDetails.isLockAcquired()) {`
`69`	`69`	`objectLocks.addAll(keys);`
	`70`	`+ this.lockDetails = OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED;`
	`71`	`+ } else {`
	`72`	`+ this.lockDetails = OMLockDetails.EMPTY_DETAILS_LOCK_NOT_ACQUIRED;`
`70`	`73`	`}`
`71`		`- this.lockDetails = omLockDetails;`
`72`	`74`	`return omLockDetails;`
`73`	`75`	`}`
`74`	`76`
`@@ -78,6 +80,8 @@ public synchronized void releaseLock() {`
`78`	`80`	`} else {`
`79`	`81`	`lockDetails = lock.releaseReadLocks(resource, this.objectLocks);`
`80`	`82`	`}`
	`83`	`+ this.lockDetails = lockDetails.isLockAcquired() ? OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED :`
	`84`	`+ OMLockDetails.EMPTY_DETAILS_LOCK_NOT_ACQUIRED;`
`81`	`85`	`this.objectLocks.clear();`
`82`	`86`	`}`
`83`	`87`