github-actions[bot] commented on code in PR #64765:
URL: https://github.com/apache/doris/pull/64765#discussion_r3642911242
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalDatabase.java:
##########
@@ -121,13 +121,22 @@ public void resetMetaToUninitialized() {
LOG.debug("resetToUninitialized db name {}, id {}, isInitializing:
{}, initialized: {}",
this.name, this.id, isInitializing, initialized, new
Exception());
}
+ boolean needInvalidateMetaCache = false;
synchronized (this) {
this.initialized = false;
this.lowerCaseToTableName = Maps.newConcurrentMap();
if (metaCache != null) {
- metaCache.invalidateAll();
+ needInvalidateMetaCache = true;
}
}
+ // Break AB-BA deadlock between synchronized(this) and MetaCache
internal CHM lock.
+ // metaCache.invalidateAll() is thread-safe by itself (Caffeine
internal locking).
+ // Moving it out of synchronized(this) prevents the circular
dependency:
+ // Thread A: synchronized(this) -> metaCache.invalidateAll() -> CHM
lock
+ // Thread B: CHM lock -> cache loader -> synchronized(this)
+ if (needInvalidateMetaCache) {
Review Comment:
[P1] Fence in-flight loads before invalidating outside the monitor
This fixes the lock cycle, but direct `REFRESH DATABASE` keeps this same
`ExternalDatabase`/`MetaCache`. A miss that already holds a Caffeine bin can
resume when this monitor is released and publish after `invalidateAll()`'s weak
traversal has passed that bin; Caffeine 2.9.3 explicitly leaves bulk
invalidation of an otherwise-absent in-flight load undefined. The refresh can
therefore return with the pre-refresh table object still cached (while
`idToName` was cleared). Please detach/swap the cache generation under the
monitor or add an invalidation-generation fence so pre-reset loads cannot
populate the post-reset cache, and cover the race with a real `MetaCache`
assertion rather than only the synthetic lock.
##########
fe/fe-core/src/test/java/org/apache/doris/datasource/metacache/MetaCacheCrossLockDeadlockTest.java:
##########
@@ -0,0 +1,228 @@
+// 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.metacache;
+
+import org.apache.doris.catalog.Env;
+import org.apache.doris.common.Pair;
+import org.apache.doris.datasource.ExternalMetaCacheMgr;
+import org.apache.doris.datasource.test.TestExternalCatalog;
+import org.apache.doris.datasource.test.TestExternalDatabase;
+import org.apache.doris.datasource.test.TestExternalTable;
+
+import com.github.benmanes.caffeine.cache.CacheLoader;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+
+import java.lang.reflect.Field;
+import java.util.List;
+import java.util.Optional;
+import java.util.OptionalLong;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.concurrent.locks.ReentrantLock;
+
+/**
+ * Regression test for cross-lock deadlock (issue 1675416).
+ *
+ * <p>Two locks are involved in production:
+ * <ul>
+ * <li><b>A</b> = {@code ExternalDatabase} instance monitor.</li>
+ * <li><b>B</b> = a CHM bin lock inside the db-level {@code MetaCache}
+ * (held by Caffeine while running the cache loader / removal
listener).</li>
+ * </ul>
+ *
+ * <p>The deadlock occurs when:
+ * <pre>
+ * T1 (refresh): holds A inside resetMetaToUninitialized, then waits for B
+ * because metaCache.invalidateAll() needs the bin lock.
+ * T2 (query): holds B inside the cache loader, then waits for A
+ * because the loader calls makeSureInitialized().
+ * </pre>
+ *
+ * <p>Fix: move {@code metaCache.invalidateAll()} <em>out</em> of
+ * {@code synchronized(this)} in {@code
ExternalDatabase#resetMetaToUninitialized()}.
+ *
+ * <p>This UT does <b>not</b> rely on Caffeine's internal CHM bin lock (which
+ * makes timing brittle). Instead it substitutes the db's {@code MetaCache}
+ * with a subclass whose {@code invalidateAll()} acquires an external lock
+ * <b>B</b>. Then we orchestrate exactly the AB-BA topology:
+ * <ul>
+ * <li>T1 holds B and waits for A (mimics the loader path).</li>
+ * <li>T2 calls {@code resetMetaToUninitialized()} (which would acquire A
+ * and then call {@code invalidateAll()} that needs B).</li>
+ * </ul>
+ *
+ * <p>With the fix, T2 releases A before calling {@code invalidateAll()}, so
+ * T1 can finish; without the fix the test deterministically deadlocks.
+ */
+public class MetaCacheCrossLockDeadlockTest {
+
+ @Test(timeout = 30_000)
+ public void testNoDeadlockBetweenResetAndInvalidateAll() throws Exception {
+ // ----- Mock catalog (avoid pulling in Env's catalog wiring) -----
+ TestExternalCatalog catalog = Mockito.mock(TestExternalCatalog.class);
+ Mockito.when(catalog.getId()).thenReturn(1001L);
+ Mockito.when(catalog.getName()).thenReturn("test_catalog");
+ Mockito.when(catalog.getLowerCaseTableNames()).thenReturn(0);
+ Mockito.when(catalog.getLowerCaseMetaNames()).thenReturn("false");
+ Mockito.when(catalog.getMetaNamesMapping()).thenReturn("");
+ Mockito.doNothing().when(catalog).makeSureInitialized();
+
+ // Real ExternalDatabase under test.
+ TestExternalDatabase db = new TestExternalDatabase(catalog, 2002L,
"test_db", "test_db");
+
+ // ----- Build a MetaCache subclass whose invalidateAll() acquires
lock B -----
+ final ReentrantLock lockB = new ReentrantLock();
+ final CountDownLatch t1HoldsB = new CountDownLatch(1);
+ final CountDownLatch t2InvalidateAllStarted = new CountDownLatch(1);
+
+ ExecutorService cacheExecutor = Executors.newSingleThreadExecutor(r ->
{
+ Thread t = new Thread(r, "metacache-deadlock-ut-cache-exec");
+ t.setDaemon(true);
+ return t;
+ });
+
+ CacheLoader<String, List<Pair<String, String>>> namesLoader =
+ key -> java.util.Collections.emptyList();
+ CacheLoader<String, Optional<TestExternalTable>> objLoader =
+ key -> Optional.empty();
+
+ MetaCache<TestExternalTable> spyCache = new
MetaCache<TestExternalTable>(
+ "deadlock-ut",
+ cacheExecutor,
+ OptionalLong.empty(),
+ OptionalLong.empty(),
+ 64,
+ namesLoader,
+ objLoader,
+ (k, v, c) -> { /* no-op */ }) {
+ @Override
+ public void invalidateAll() {
+ // Mark that T2 has reached invalidateAll so T1 can release B
in time.
+ t2InvalidateAllStarted.countDown();
+ // Acquire lock B (this is the bin-lock in production). If T2
still
+ // holds A here (i.e. fix has been reverted), and T1 holds B
waiting
+ // for A, this lock() will block forever.
+ lockB.lock();
+ try {
+ // no-op
+ } finally {
+ lockB.unlock();
+ }
+ }
+ };
+
+ // Inject MetaCache and initialized=true via reflection.
+ Field metaCacheField =
org.apache.doris.datasource.ExternalDatabase.class
+ .getDeclaredField("metaCache");
+ metaCacheField.setAccessible(true);
+ metaCacheField.set(db, spyCache);
+
+ Field initializedField =
org.apache.doris.datasource.ExternalDatabase.class
+ .getDeclaredField("initialized");
+ initializedField.setAccessible(true);
+ initializedField.setBoolean(db, true);
+
+ // ----- Mock Env so the tail-call invalidateDb is a no-op -----
+ ExternalMetaCacheMgr mgr = Mockito.mock(ExternalMetaCacheMgr.class);
+ Mockito.doNothing().when(mgr).invalidateDb(Mockito.anyLong(),
Mockito.anyString());
+ Env env = Mockito.mock(Env.class);
+ Mockito.when(env.getExtMetaCacheMgr()).thenReturn(mgr);
+
+ ExecutorService runner = Executors.newFixedThreadPool(2, r -> {
+ Thread t = new Thread(r);
+ t.setDaemon(true);
+ return t;
+ });
+
+ AtomicReference<Throwable> t1Error = new AtomicReference<>();
+ AtomicReference<Throwable> t2Error = new AtomicReference<>();
+
+ try (MockedStatic<Env> envMock = Mockito.mockStatic(Env.class,
Mockito.CALLS_REAL_METHODS)) {
Review Comment:
[P2] Keep the `Env` mock on the thread that performs the reset
Mockito static mocks only affect the thread that creates them, but this
scope is opened on the JUnit thread while `resetMetaToUninitialized()` runs in
the `runner` task below. Its tail call therefore executes the real
`Env.getCurrentEnv()` and can initialize or mutate the process-wide
catalog/cache manager (including any real catalog using ID 1001), making this
lock-order test depend on unrelated suite state and timing. Please create/close
the static mock inside the T2 worker (or use a test seam that avoids the global
static call) so the reset always reaches the local no-op manager.
--
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]
