github-actions[bot] commented on code in PR #65409:
URL: https://github.com/apache/doris/pull/65409#discussion_r3550058677
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalDatabase.java:
##########
@@ -120,13 +120,22 @@ public void resetMetaToUninitialized() {
LOG.debug("resetToUninitialized db name {}, id {}, isInitializing:
{}, initialized: {}",
this.name, this.id, isInitializing, initialized, new
Exception());
}
+ MetaCache<T> cacheToInvalidate = null;
synchronized (this) {
this.initialized = false;
this.lowerCaseToTableName = Maps.newConcurrentMap();
if (metaCache != null) {
- metaCache.invalidateAll();
+ cacheToInvalidate = metaCache;
+ metaCache = null;
Review Comment:
This fix publishes a new `null` state for `metaCache`, but callers still
dereference the field outside the database monitor after
`makeSureInitialized()` returns. For example, `getTableNullable()` does
`makeSureInitialized()` and then calls `metaCache.getMetaObj(...)`; a
concurrent `REFRESH DATABASE` can now run between those two operations, execute
this assignment, and make the query/planner thread throw an NPE.
`getTableNamesWithLock()` has the same shape before `metaCache.listNames()`.
Previously refresh invalidated the existing cache object without nulling the
field, so these readers kept a non-null reference. Please keep the old cache
reference visible until replacement is ready, or snapshot/retry under
synchronization while still moving `invalidateAll()` itself outside the monitor.
##########
fe/fe-core/src/test/java/org/apache/doris/datasource/ExternalDatabaseDeadlockTest.java:
##########
@@ -0,0 +1,308 @@
+// 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.doris.datasource;
+
+import org.apache.doris.datasource.InitCatalogLog.Type;
+
+import com.github.benmanes.caffeine.cache.Caffeine;
+import com.github.benmanes.caffeine.cache.LoadingCache;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.ThreadMXBean;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * Regression test to verify that ExternalDatabase.resetMetaToUninitialized()
+ * does not deadlock with concurrent Caffeine cache loading that calls back
+ * into ExternalDatabase.makeSureInitialized().
+ *
+ * The deadlock lock ordering (before fix):
+ * Path 1 (REFRESH DATABASE):
+ * synchronized(ExternalDatabase) -> metaCache.invalidateAll() -> Caffeine
internal locks
+ * Path 2 (cache loading):
+ * Caffeine internal locks (computeIfAbsent) -> loader ->
buildTableForInit()
+ * -> getTableNamesWithLock() -> makeSureInitialized() ->
synchronized(ExternalDatabase)
+ *
+ * After fix:
+ * - resetMetaToUninitialized() releases synchronized(this) before
invalidateAll()
+ * - getTableNamesForCheck() avoids makeSureInitialized() on fast path,
+ * with safe fallback to getTableNamesWithLock() when metaCache was reset
+ */
+public class ExternalDatabaseDeadlockTest {
+
+ @Test
+ public void testResetMetaToUninitializedShouldNotDeadlockWithCacheLoader()
throws Exception {
+ DeadlockDbCatalog catalog = new DeadlockDbCatalog();
+ DeadlockDatabase db = new DeadlockDatabase(catalog, 1L, "test_db",
"test_db");
+ // Initialize the database so that metaCache is built
+ db.makeSureInitialized();
+
+ CountDownLatch loaderEntered = new CountDownLatch(1);
+ CountDownLatch allowLoaderToTouchDb = new CountDownLatch(1);
+ AtomicReference<Throwable> backgroundFailure = new AtomicReference<>();
+
+ // The loader holds Caffeine's per-key lock before it calls back into
the database.
+ // This models what happens when MetaCache.getMetaObj() triggers the
metaObjCache loader:
+ // Caffeine internal lock -> loader -> buildTableForInit ->
makeSureInitialized
+ LoadingCache<String, String> cache = Caffeine.newBuilder().build(key
-> {
+ loaderEntered.countDown();
+ awaitLatch(allowLoaderToTouchDb);
+ // This simulates the callback from cache loader into the database:
+ // buildTableForInit -> getTableNamesWithLock ->
makeSureInitialized
+ db.makeSureInitialized();
+ return key;
+ });
+
+ // Thread B: cache loader thread (holds Caffeine node lock)
+ Thread queryThread = new Thread(
+ () -> runQuietly(backgroundFailure, () ->
cache.get("deadlock-key")),
+ "deadlock-db-cache-loader");
+ queryThread.setDaemon(true);
+ queryThread.start();
+ Assertions.assertTrue(loaderEntered.await(5, TimeUnit.SECONDS),
+ "loader should have entered within timeout");
+
+ // Thread A: refresh database thread
+ // With the fix, resetMetaToUninitialized() releases
synchronized(this) before
+ // calling invalidateAll(). We validate by: (1) taking the db monitor
briefly
+ // to update state, (2) releasing the monitor, (3) then invalidating
the cache
+ // which needs Caffeine internal locks. This must not deadlock with
Thread B.
+ Thread refreshThread = new Thread(
+ () -> runQuietly(backgroundFailure, () -> {
+ // Simulate the internal behavior of
resetMetaToUninitialized():
+ // Step 1: synchronized(this) { update state }
+ synchronized (db) {
+ db.setInitializedForTest(false);
+ }
+ // Step 2: allow the loader to proceed (releasing the
monitor first)
+ allowLoaderToTouchDb.countDown();
+ // Step 3: invalidate cache outside the lock (the fix)
+ // This should not deadlock even though Thread B holds
Caffeine node lock
+ // and is trying to acquire synchronized(db)
+ cache.invalidate("deadlock-key");
+ }),
+ "deadlock-db-refresh");
+ refreshThread.setDaemon(true);
+ refreshThread.start();
+
+ assertNoDeadlock(queryThread, refreshThread, backgroundFailure);
+ }
+
+ /**
+ * Test the actual fix: resetMetaToUninitialized() with a concurrent cache
loader
+ * that goes through the real buildTableForInit -> getTableNamesForCheck
path.
+ */
+ @Test
+ public void testResetMetaToUninitializedWithRealBuildTableForInitPath()
throws Exception {
+ DeadlockDbCatalog catalog = new DeadlockDbCatalog();
+ CoordinatedDatabase db = new CoordinatedDatabase(catalog, 1L,
"test_db", "test_db");
+ db.makeSureInitialized();
+
+ CountDownLatch loaderEntered = new CountDownLatch(1);
+ CountDownLatch allowLoaderToProceed = new CountDownLatch(1);
+ db.setLatches(loaderEntered, allowLoaderToProceed);
+
+ AtomicReference<Throwable> backgroundFailure = new AtomicReference<>();
+
+ // Thread B: triggers getTableNullable which eventually triggers the
+ // metaObjCache loader -> buildTableForInit -> getTableNamesForCheck
+ Thread queryThread = new Thread(
+ () -> runQuietly(backgroundFailure, () -> {
+ db.getTableNullable("test_table");
+ }),
+ "deadlock-db-real-loader");
+ queryThread.setDaemon(true);
+ queryThread.start();
+ Assertions.assertTrue(loaderEntered.await(10, TimeUnit.SECONDS),
+ "buildTableForInit should have been entered within timeout");
+
+ // Thread A: call the real resetMetaToUninitialized()
+ Thread refreshThread = new Thread(
+ () -> runQuietly(backgroundFailure, () -> {
+ db.resetMetaToUninitialized();
+ allowLoaderToProceed.countDown();
+ }),
+ "deadlock-db-real-refresh");
+ refreshThread.setDaemon(true);
+ refreshThread.start();
+
+ assertNoDeadlock(queryThread, refreshThread, backgroundFailure);
+ }
+
+ // ---- Test harness ----
+
+ private static void assertNoDeadlock(Thread t1, Thread t2,
+ AtomicReference<Throwable> backgroundFailure) throws Exception {
+ long[] deadlockedThreads = waitForDeadlock(t1, t2);
+ t1.join(TimeUnit.SECONDS.toMillis(10));
+ t2.join(TimeUnit.SECONDS.toMillis(10));
+ Assertions.assertNull(backgroundFailure.get(),
+ "unexpected background failure: " + backgroundFailure.get());
+ Assertions.assertNull(deadlockedThreads,
+ String.format("detected deadlock between threads %s and %s",
+ t1.getName(), t2.getName()));
+ Assertions.assertFalse(t1.isAlive(), t1.getName() + " is still
running");
+ Assertions.assertFalse(t2.isAlive(), t2.getName() + " is still
running");
+ }
+
+ private static void awaitLatch(CountDownLatch latch) throws
InterruptedException {
+ Assertions.assertTrue(latch.await(10, TimeUnit.SECONDS));
+ }
+
+ private static void runQuietly(AtomicReference<Throwable> failure,
ThrowingRunnable task) {
+ try {
+ task.run();
+ } catch (Throwable t) {
+ failure.compareAndSet(null, t);
+ }
+ }
+
+ private static long[] waitForDeadlock(Thread t1, Thread t2) throws
InterruptedException {
+ ThreadMXBean threadMxBean = ManagementFactory.getThreadMXBean();
+ long t1Id = t1.threadId();
Review Comment:
This test will not compile with the FE Java 17 target. `fe/pom.xml` sets
`maven.compiler.source`/`target` to 17, but `Thread.threadId()` is only
available on newer JDK APIs; a `javac --release 17` probe fails with `cannot
find symbol: method threadId()`. Please use `Thread.getId()` here and below so
the FE test suite can compile.
##########
fe/fe-core/src/test/java/org/apache/doris/datasource/ExternalDatabaseDeadlockTest.java:
##########
@@ -0,0 +1,308 @@
+// 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.doris.datasource;
+
+import org.apache.doris.datasource.InitCatalogLog.Type;
+
+import com.github.benmanes.caffeine.cache.Caffeine;
+import com.github.benmanes.caffeine.cache.LoadingCache;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.ThreadMXBean;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * Regression test to verify that ExternalDatabase.resetMetaToUninitialized()
+ * does not deadlock with concurrent Caffeine cache loading that calls back
+ * into ExternalDatabase.makeSureInitialized().
+ *
+ * The deadlock lock ordering (before fix):
+ * Path 1 (REFRESH DATABASE):
+ * synchronized(ExternalDatabase) -> metaCache.invalidateAll() -> Caffeine
internal locks
+ * Path 2 (cache loading):
+ * Caffeine internal locks (computeIfAbsent) -> loader ->
buildTableForInit()
+ * -> getTableNamesWithLock() -> makeSureInitialized() ->
synchronized(ExternalDatabase)
+ *
+ * After fix:
+ * - resetMetaToUninitialized() releases synchronized(this) before
invalidateAll()
+ * - getTableNamesForCheck() avoids makeSureInitialized() on fast path,
+ * with safe fallback to getTableNamesWithLock() when metaCache was reset
+ */
+public class ExternalDatabaseDeadlockTest {
+
+ @Test
+ public void testResetMetaToUninitializedShouldNotDeadlockWithCacheLoader()
throws Exception {
+ DeadlockDbCatalog catalog = new DeadlockDbCatalog();
+ DeadlockDatabase db = new DeadlockDatabase(catalog, 1L, "test_db",
"test_db");
+ // Initialize the database so that metaCache is built
+ db.makeSureInitialized();
+
+ CountDownLatch loaderEntered = new CountDownLatch(1);
+ CountDownLatch allowLoaderToTouchDb = new CountDownLatch(1);
+ AtomicReference<Throwable> backgroundFailure = new AtomicReference<>();
+
+ // The loader holds Caffeine's per-key lock before it calls back into
the database.
+ // This models what happens when MetaCache.getMetaObj() triggers the
metaObjCache loader:
+ // Caffeine internal lock -> loader -> buildTableForInit ->
makeSureInitialized
+ LoadingCache<String, String> cache = Caffeine.newBuilder().build(key
-> {
+ loaderEntered.countDown();
+ awaitLatch(allowLoaderToTouchDb);
+ // This simulates the callback from cache loader into the database:
+ // buildTableForInit -> getTableNamesWithLock ->
makeSureInitialized
+ db.makeSureInitialized();
+ return key;
+ });
+
+ // Thread B: cache loader thread (holds Caffeine node lock)
+ Thread queryThread = new Thread(
+ () -> runQuietly(backgroundFailure, () ->
cache.get("deadlock-key")),
+ "deadlock-db-cache-loader");
+ queryThread.setDaemon(true);
+ queryThread.start();
+ Assertions.assertTrue(loaderEntered.await(5, TimeUnit.SECONDS),
+ "loader should have entered within timeout");
+
+ // Thread A: refresh database thread
+ // With the fix, resetMetaToUninitialized() releases
synchronized(this) before
+ // calling invalidateAll(). We validate by: (1) taking the db monitor
briefly
+ // to update state, (2) releasing the monitor, (3) then invalidating
the cache
+ // which needs Caffeine internal locks. This must not deadlock with
Thread B.
+ Thread refreshThread = new Thread(
+ () -> runQuietly(backgroundFailure, () -> {
+ // Simulate the internal behavior of
resetMetaToUninitialized():
+ // Step 1: synchronized(this) { update state }
+ synchronized (db) {
+ db.setInitializedForTest(false);
+ }
+ // Step 2: allow the loader to proceed (releasing the
monitor first)
+ allowLoaderToTouchDb.countDown();
+ // Step 3: invalidate cache outside the lock (the fix)
+ // This should not deadlock even though Thread B holds
Caffeine node lock
+ // and is trying to acquire synchronized(db)
+ cache.invalidate("deadlock-key");
+ }),
+ "deadlock-db-refresh");
+ refreshThread.setDaemon(true);
+ refreshThread.start();
+
+ assertNoDeadlock(queryThread, refreshThread, backgroundFailure);
+ }
+
+ /**
+ * Test the actual fix: resetMetaToUninitialized() with a concurrent cache
loader
+ * that goes through the real buildTableForInit -> getTableNamesForCheck
path.
+ */
+ @Test
+ public void testResetMetaToUninitializedWithRealBuildTableForInitPath()
throws Exception {
+ DeadlockDbCatalog catalog = new DeadlockDbCatalog();
+ CoordinatedDatabase db = new CoordinatedDatabase(catalog, 1L,
"test_db", "test_db");
+ db.makeSureInitialized();
+
+ CountDownLatch loaderEntered = new CountDownLatch(1);
+ CountDownLatch allowLoaderToProceed = new CountDownLatch(1);
+ db.setLatches(loaderEntered, allowLoaderToProceed);
+
+ AtomicReference<Throwable> backgroundFailure = new AtomicReference<>();
+
+ // Thread B: triggers getTableNullable which eventually triggers the
+ // metaObjCache loader -> buildTableForInit -> getTableNamesForCheck
+ Thread queryThread = new Thread(
+ () -> runQuietly(backgroundFailure, () -> {
+ db.getTableNullable("test_table");
+ }),
+ "deadlock-db-real-loader");
+ queryThread.setDaemon(true);
+ queryThread.start();
+ Assertions.assertTrue(loaderEntered.await(10, TimeUnit.SECONDS),
+ "buildTableForInit should have been entered within timeout");
+
+ // Thread A: call the real resetMetaToUninitialized()
+ Thread refreshThread = new Thread(
+ () -> runQuietly(backgroundFailure, () -> {
+ db.resetMetaToUninitialized();
Review Comment:
This ordering makes the test timing-dependent. The query thread is already
inside the `MetaCache.getMetaObj()` loader and waits on
`allowLoaderToProceed.await(10, TimeUnit.SECONDS)`, but the refresh thread only
counts that latch down after `resetMetaToUninitialized()` returns. Since reset
synchronously invalidates the old `MetaCache`, it can be waiting on the same
Caffeine loader that is waiting for this latch; the only remaining progress
path is the 10-second timeout in the loader override. That makes every run slow
and no longer proves the fixed lock ordering deterministically. Please release
the loader from a deterministic point before waiting for reset to return, and
assert the latch result rather than ignoring the timeout.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
