apache / apache/datafusion-comet

Rust UDF library cache holds its write lock across dlopen

Open
#5,297 0 comments 0 reactions 0 assignees View on GitHub
enhancement performance
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.