This is an automated email from the ASF dual-hosted git repository.
roryqi 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 34dcfa1fa8 [#9084] improvement(core): Fix race condition in
AuthorizationRequestContext.loadRole() (#9086)
34dcfa1fa8 is described below
commit 34dcfa1fa879e2f25e5e346b382b50ea26b4f6d4
Author: Harsh Mehta <[email protected]>
AuthorDate: Fri Dec 5 14:23:29 2025 +0530
[#9084] improvement(core): Fix race condition in
AuthorizationRequestContext.loadRole() (#9086)
Fixed #9084
- Replaced separate `hasLoadRole.get()` and `hasLoadRole.set(true)`
calls with an atomic
`hasLoadRole.compareAndSet(false, true)` operation.
- This makes the check-and-set atomic, ensuring the runnable executes
only once,
even under concurrent invocations.
- Previously, multiple threads could see `false` and execute the
runnable simultaneously,
causing duplicate executions.
- Added a new unit test:
`testLoadRoleRunsOnceEvenWhenInvokedConcurrently`.
- Verified that the runnable executes only once, even when `loadRole()`
is called concurrently.
### Result
Improved thread-safety of `loadRole()` and prevented race conditions
during concurrent access.
---------
Signed-off-by: harsh-mehta-sb <[email protected]>
Signed-off-by: Harsh Mehta <[email protected]>
---
.../authorization/AuthorizationRequestContext.java | 13 ++-
.../TestAuthorizationRequestContext.java | 104 +++++++++++++++++++++
2 files changed, 115 insertions(+), 2 deletions(-)
diff --git
a/core/src/main/java/org/apache/gravitino/authorization/AuthorizationRequestContext.java
b/core/src/main/java/org/apache/gravitino/authorization/AuthorizationRequestContext.java
index bc8964ccfa..392eb13c27 100644
---
a/core/src/main/java/org/apache/gravitino/authorization/AuthorizationRequestContext.java
+++
b/core/src/main/java/org/apache/gravitino/authorization/AuthorizationRequestContext.java
@@ -82,8 +82,17 @@ public class AuthorizationRequestContext {
if (hasLoadRole.get()) {
return;
}
- runnable.run();
- hasLoadRole.set(true);
+ synchronized (this) {
+ if (hasLoadRole.get()) {
+ return;
+ }
+ try {
+ runnable.run();
+ hasLoadRole.set(true);
+ } catch (Exception e) {
+ throw new RuntimeException("Failed to load role: ", e);
+ }
+ }
}
public String getOriginalAuthorizationExpression() {
diff --git
a/core/src/test/java/org/apache/gravitino/authorization/TestAuthorizationRequestContext.java
b/core/src/test/java/org/apache/gravitino/authorization/TestAuthorizationRequestContext.java
new file mode 100644
index 0000000000..2216a7dcfe
--- /dev/null
+++
b/core/src/test/java/org/apache/gravitino/authorization/TestAuthorizationRequestContext.java
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.authorization;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.junit.jupiter.api.Test;
+
+public class TestAuthorizationRequestContext {
+
+ @Test
+ public void testLoadRoleRunsOnceEvenWhenInvokedConcurrently() throws
Exception {
+ AuthorizationRequestContext context = new AuthorizationRequestContext();
+ AtomicInteger counter = new AtomicInteger();
+ CountDownLatch firstStarted = new CountDownLatch(1);
+ CountDownLatch allowFinish = new CountDownLatch(1);
+
+ Thread firstInvocation =
+ new Thread(
+ () ->
+ context.loadRole(
+ () -> {
+ counter.incrementAndGet();
+ firstStarted.countDown();
+ try {
+ allowFinish.await(5, TimeUnit.SECONDS);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ }));
+ firstInvocation.start();
+
+ try {
+ assertTrue(firstStarted.await(5, TimeUnit.SECONDS));
+ context.loadRole(counter::incrementAndGet);
+ assertEquals(1, counter.get());
+ } finally {
+ allowFinish.countDown();
+ firstInvocation.join();
+ }
+
+ context.loadRole(counter::incrementAndGet);
+ assertEquals(1, counter.get(), "Subsequent loadRole calls should be
ignored");
+ }
+
+ @Test
+ public void testLoadRoleFailThenSuccessThenIgnored() throws Exception {
+ AuthorizationRequestContext context = new AuthorizationRequestContext();
+ AtomicInteger counter = new AtomicInteger();
+
+ CountDownLatch failingStarted = new CountDownLatch(1);
+ CountDownLatch allowFailToThrow = new CountDownLatch(1);
+
+ Thread failingThread =
+ new Thread(
+ () -> {
+ try {
+ context.loadRole(
+ () -> {
+ counter.incrementAndGet();
+ failingStarted.countDown();
+ try {
+ allowFailToThrow.await(2, TimeUnit.SECONDS);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ throw new IllegalStateException("Simulated failure");
+ });
+ } catch (RuntimeException e) {
+ assertTrue(e.getMessage().contains("Failed to load role"));
+ assertInstanceOf(IllegalStateException.class, e.getCause());
+ }
+ });
+
+ failingThread.start();
+ assertTrue(failingStarted.await(2, TimeUnit.SECONDS));
+ allowFailToThrow.countDown();
+ failingThread.join();
+ context.loadRole(counter::incrementAndGet);
+ assertEquals(2, counter.get(), "Flag should remain false after failure so
next call runs.");
+ context.loadRole(counter::incrementAndGet);
+ assertEquals(2, counter.get(), "After a successful loadRole, further calls
must be ignored.");
+ }
+}
