leaves12138 commented on PR #8003:
URL: https://github.com/apache/paimon/pull/8003#issuecomment-4561053851
I did another thread-safety focused pass on the latest revision
(`a85568af6e`). The mutable `KeySerializer` buffer issue looks fixed now, and
the BTree reader path mostly looks safe: each range query creates its own
`SstFileIterator` / `BlockIterator`, `BlockCache` uses a concurrent map and
synchronizes non-vectored stream access, and `LazyField` uses double-checked
locking for the null bitmap.
However, I found one remaining concurrency/liveness issue that I think
should be fixed before merging.
`LazyFilteredBTreeReader.visitParallel` nests async submissions onto the
same executor:
```java
CompletableFuture.supplyAsync(() -> getOrCreateReader(meta), executor)
.thenCompose(visitor)
```
and each `visitor` call eventually enters `BTreeIndexReader.supplyResult`,
which does another `CompletableFuture.supplyAsync(..., executor)`.
This is risky with the executor returned by
`GlobalIndexReadThreadPool.getExecutorService(threadNum)`. When `threadNum` is
smaller than the shared pool max, it returns a `SemaphoredDelegatingExecutor`,
whose `execute/submit` acquires permits synchronously. With
`global-index.thread-num = 1`, the first stage may still be holding the only
permit when the `thenCompose` callback tries to submit the second stage to the
same executor, so the worker thread blocks acquiring a permit that it can only
release after the callback returns. That can deadlock the query.
A minimal reproduction of the executor pattern is:
```java
ExecutorService base = ThreadPoolUtils.createCachedThreadPool(4, "demo");
ExecutorService executor = new SemaphoredDelegatingExecutor(base, 1, false);
CountDownLatch latch = new CountDownLatch(1);
CompletableFuture<String> outer = CompletableFuture.supplyAsync(() -> {
try {
latch.await();
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
return "outer";
}, executor);
CompletableFuture<String> f = outer.thenCompose(
v -> CompletableFuture.supplyAsync(() -> "inner", executor));
latch.countDown();
f.get(2, TimeUnit.SECONDS); // TimeoutException
```
I also ran the current BTree concurrency tests locally and they pass:
```bash
mvn -pl paimon-common -Pfast-build
-Dtest=BTreeThreadSafetyTest,LazyFilteredBTreeIndexReaderTest test
```
but those tests use regular fixed thread pools / direct executors, so they
do not cover the `SemaphoredDelegatingExecutor` path used when users configure
a smaller `global-index.thread-num`.
Possible fixes:
1. Avoid the nested async hop in `LazyFilteredBTreeReader`, e.g. create/get
the reader synchronously and call the visitor directly in a single
`supplyAsync` stage, or
2. make `BTreeIndexReader` expose synchronous read methods internally so
`LazyFilteredBTreeReader` owns exactly one async submission per selected file,
or
3. add a test using `GlobalIndexReadThreadPool.getExecutorService(1)` /
`SemaphoredDelegatingExecutor` to prevent regressions.
--
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]
