andygrove opened a new issue, #5297: URL: https://github.com/apache/datafusion-comet/issues/5297
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<HashMap<PathBuf, Arc<LoadedLibrary>>>` 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. -- 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]
