apache / apache/datafusion-comet
Rust UDF library cache holds its write lock across dlopen
- Dominant language
- Scala
- Stars
- 1.3k
- Forks
- 373
- Avg merge
- 2d 6h
- Merged PRs (30d)
- 190
Description
Follow-up from review of #4459 ([thread](https://github.com/apache/datafusion-comet/pull/4459#discussion_r3730406284)), raised by @mbutrovich.
`cache::get_or_load` holds the cache's single write lock across `load(&canonical)`, which runs `Library::new` (and with it the cdylib's static initializers) plus the discovery routine. The cache is one process-wide `RwLock>>` rather than sharded per path, so a slow load of one library blocks `get_or_load` for every unrelated library path in the process for as long as that load takes.
There is a second-order effect worth fixing in the same change: if anything under that lock panics, the `RwLock` is poisoned and every subsequent `.unwrap()` in the cache panics for the life of the process — so one bad library disables Rust UDFs entirely rather than failing only its own query.
Impact is bounded in practice (one load per library per process, and `dlopen` is fast), which is why it was left out of #4459.
The obvious shape — load outside the lock, double-check on insert — needs care: a race can produce two `LoadedLibrary` values for one path, and dropping the loser performs a `dlclose` that the current design deliberately never does (see the comment at the top of `cache.rs` and the field ordering in `LoadedLibrary`). Per-path locking, or a `OnceCell` per entry, avoids both.
Contributor guide
Research direction
Start in cache.rs at cache::get_or_load, the top-level cache comment, and LoadedLibrary's field ordering; trace how Library::new and discovery run under the process-wide lock. Review the existing cache tests, if present, and add coverage for concurrent unrelated loads and recovery after a failed load. Done means one path's load does not block unrelated paths and a failure does not poison future cache operations.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100