JingsongLi commented on PR #1186: URL: https://github.com/apache/paimon-rust/pull/1186#issuecomment-6312645253
**[P2] Avoid retaining the block lock on an idle caller runtime** In [reader.rs:115–120](https://github.com/apache/paimon-rust/blob/0bcca96435320b43553abd3999875cb696d82859/crates/paimon/src/io/cache/reader.rs#L115-L120), the detached fill owns `load_guard` after the foreground read has returned. With a Tokio current-thread runtime, `block_on` returning stops driving that task. If runtime A remains alive but idle, a subsequent read of the same block through the same cache on runtime B waits indefinitely at `load_lock.lock_owned().await`. The shared file lock's two-second timeout cannot help, because the process-local mutex is acquired first. This introduces a runtime-lifetime dependency after a completed read; the previous synchronous publication released the guard before returning. **Reproduction** 1. Add the following test inside the existing `tests` module in `crates/paimon/src/io/cache/reader.rs`. It uses that module's existing `CountingReader` and imports. The small block size keeps the example minimal; shared mode's default 1 MiB blocks also select this background-fill path. This unit test constructs `CachedFileReader` directly; when reproducing through `FileIO`, use an eligible metadata path or include `data` in `local-cache.whitelist`. ```rust #[test] fn review_1186_cross_runtime_read_does_not_stall_behind_idle_fill() { let directory = tempfile::tempdir().unwrap(); let delegate = Arc::new(CountingReader { data: Bytes::from_static(b"abcdefgh"), reads: AtomicUsize::new(0), }); let cache = Arc::new( LocalCache::new(LocalCacheConfig { shared: true, dir: Some(directory.path().to_path_buf()), namespace: "test".into(), max_size: Some(4096), block_size: 4, whitelist: std::collections::HashSet::from([FileType::Meta]), }) .unwrap(), ); let reader = CachedFileReader::new(delegate.clone(), "data.parquet", 8, cache); let first_runtime = tokio::runtime::Builder::new_current_thread() .enable_all() .build() .unwrap(); assert_eq!(first_runtime.block_on(reader.read(0..4)).unwrap(), "abcd"); // Keep the first runtime alive, but stop driving it. let second_runtime = tokio::runtime::Builder::new_current_thread() .enable_all() .build() .unwrap(); let result = second_runtime.block_on(async { tokio::time::timeout(std::time::Duration::from_millis(500), reader.read(0..4)).await }); assert!( result.is_ok(), "a completed read must not hold a block lock on its idle caller runtime" ); assert_eq!(result.unwrap().unwrap(), "abcd"); } ``` 2. Run: ```sh cargo +1.94.0 test --locked -p paimon --lib \ --no-default-features --features storage-fs,storage-memory -j4 \ review_1186_cross_runtime_read_does_not_stall_behind_idle_fill ``` Expected: both reads finish successfully. Actual on `0bcca964`: the second read hits the 500 ms test timeout; without the timeout it remains blocked while runtime A stays alive and idle. No artificial directory-lock contention is required. Verified on macOS with Rust 1.94.0: the identical test passes on base `e4ec3a45` and fails on head `0bcca964`. The full `io::cache` filter on that head passes all 81 existing tests, with only this added regression test failing. **Suggested fix** A minimal fix is to keep publication synchronous when the caller uses `RuntimeFlavor::CurrentThread`. Alternatively, schedule fills on a cache-owned executor that is guaranteed to keep running independently of the caller's `block_on`. Preserve same-block single-flight coordination, but ensure that a completed foreground read cannot leave later reads dependent on an idle caller runtime. Please retain a cross-runtime regression test for this case. -- 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]
