(gravitino) branch main updated: [#9139] fix(lock): Prevent NPE when timestamp is missing in TreeLock.unlock (#9146)

[jmclean]

This is an automated email from the ASF dual-hosted git repository.

jmclean pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git


The following commit(s) were added to refs/heads/main by this push:
     new 320637a3e4 [#9139] fix(lock): Prevent NPE when timestamp is missing in 
TreeLock.unlock (#9146)
320637a3e4 is described below

commit 320637a3e4976691a77cc801decc15706de442e9
Author: Kwon Taeheon <pcloud63...@gmail.com>
AuthorDate: Tue Nov 18 06:49:28 2025 +0900

    [#9139] fix(lock): Prevent NPE when timestamp is missing in TreeLock.unlock 
(#9146)
    
    ### What changes were proposed in this pull request?
    - Updated TreeLockNode.removeHoldingThreadTimestamp to return a nullable
    Long instead of long.
    - Modified TreeLock.unlock() to handle null timestamps safely before
    logging.
    - Ensures unlocking works correctly even when the timestamp map is
    cleared.
    
    ### Why are the changes needed?
    
    - holdingThreadTimestamp is a Map<ThreadIdentifier, Long>, and
    Map.remove() returns null when no timestamp exists.
    - The previous implementation returned long, causing a null → primitive
    unboxing and leading to a NullPointerException.
    - Unlocking a node should not depend on the presence of a timestamp
    value, so the logic must tolerate null entries.
    
    Fix: #9139
    
    ### Does this PR introduce _any_ user-facing change?
    
    - no.
    - This change only improves internal lock handling and exception safety.
    
    ### How was this patch tested?
    - Verified using the unit test included in the issue
    (testUnlockWithMissingHoldingTimestamp).
    - Confirmed all existing tests continue to pass.
---
 core/src/main/java/org/apache/gravitino/lock/TreeLock.java |  7 +++++--
 .../main/java/org/apache/gravitino/lock/TreeLockNode.java  |  2 +-
 .../test/java/org/apache/gravitino/lock/TestTreeLock.java  | 14 ++++++++++++++
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/core/src/main/java/org/apache/gravitino/lock/TreeLock.java 
b/core/src/main/java/org/apache/gravitino/lock/TreeLock.java
index 9ad2565dd3..a9c2d7013a 100644
--- a/core/src/main/java/org/apache/gravitino/lock/TreeLock.java
+++ b/core/src/main/java/org/apache/gravitino/lock/TreeLock.java
@@ -153,15 +153,18 @@ public class TreeLock {
       LockType type = pair.getRight();
       current.unlock(type);
 
-      long holdStartTime = 
current.removeHoldingThreadTimestamp(Thread.currentThread(), identifier);
+      Long holdStartTime = 
current.removeHoldingThreadTimestamp(Thread.currentThread(), identifier);
       if (LOG.isTraceEnabled()) {
+        long duration =
+            (holdStartTime == null) ? -1L : (System.currentTimeMillis() - 
holdStartTime);
+
         LOG.trace(
             "Node {} has been unlock with '{}' lock, hold by {} with ident 
'{}' for {} ms",
             this,
             type,
             Thread.currentThread(),
             identifier,
-            System.currentTimeMillis() - holdStartTime);
+            duration);
       }
     }
 
diff --git a/core/src/main/java/org/apache/gravitino/lock/TreeLockNode.java 
b/core/src/main/java/org/apache/gravitino/lock/TreeLockNode.java
index f9434eb7cc..599c6e2b90 100644
--- a/core/src/main/java/org/apache/gravitino/lock/TreeLockNode.java
+++ b/core/src/main/java/org/apache/gravitino/lock/TreeLockNode.java
@@ -117,7 +117,7 @@ public class TreeLockNode {
     holdingThreadTimestamp.put(ThreadIdentifier.of(currentThread, identifier), 
timestamp);
   }
 
-  long removeHoldingThreadTimestamp(Thread currentThread, NameIdentifier 
identifier) {
+  Long removeHoldingThreadTimestamp(Thread currentThread, NameIdentifier 
identifier) {
     return holdingThreadTimestamp.remove(ThreadIdentifier.of(currentThread, 
identifier));
   }
 
diff --git a/core/src/test/java/org/apache/gravitino/lock/TestTreeLock.java 
b/core/src/test/java/org/apache/gravitino/lock/TestTreeLock.java
index 2ff7c4e8e4..c48d5cf76a 100644
--- a/core/src/test/java/org/apache/gravitino/lock/TestTreeLock.java
+++ b/core/src/test/java/org/apache/gravitino/lock/TestTreeLock.java
@@ -26,6 +26,7 @@ import static org.mockito.Mockito.doThrow;
 
 import java.util.Arrays;
 import java.util.List;
+import org.apache.gravitino.NameIdentifier;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.mockito.Mockito;
@@ -109,4 +110,17 @@ class TestTreeLock {
     lock.unlock();
     assertThrows(IllegalStateException.class, () -> lock.unlock());
   }
+
+  @Test
+  void testUnlockWithMissingHoldingTimestamp() {
+    TreeLockNode rootNode = new TreeLockNode("root");
+    TreeLockNode childNode = rootNode.getOrCreateChild("child").getLeft();
+    TreeLock treeLock =
+        new TreeLock(Arrays.asList(rootNode, childNode), 
NameIdentifier.of("root", "child"));
+
+    treeLock.lock(LockType.WRITE);
+    childNode.getHoldingThreadTimestamp().clear();
+
+    assertDoesNotThrow(treeLock::unlock);
+  }
 }