Repository navigation
fix(namespace): avoid write starvation during high read-count - #70
Merged
Merged
Conversation
(cherry picked from commit 996affe)
zehiko
approved these changes
Sep 2, 2026
amunra
added
lance fork 10.0.0
zhangyue19921010
pushed a commit
to lance-format/lance
that referenced
this pull request
Sep 30, 2026
## Problem `DatasetConsistencyWrapper` (the cached manifest dataset behind the directory namespace) guards its `Dataset` with a `tokio::sync::RwLock` and exposes `get()` / `get_refreshed()` for readers and `get_mut()` for writers. `get_mut()` called `reload()` and only then requested the write lock. `reload()` itself takes a read lock to probe for a successor version and releases it before anything else happens, so from the lock's point of view the writer was just another reader until the moment it finally called `.write().await`. tokio's `RwLock` is write-preferring: once a writer is queued, no new readers are admitted. That protection never engaged for `get_mut()` while it was reloading, because during that phase it held only a read lock and no write request was queued yet. Every reader that arrived while `get_mut()` was still reloading was admitted alongside it, and the writer then had to wait for all of them before its write request was served; the fairness guarantee only covered the tail of the operation. `get_mut()` is a public method of the wrapper; the namespace's own write operations currently go through `get_refreshed()` and the copy-on-write manifest rewrite path, so this affects external callers of `get_mut()` rather than any in-tree operation. ## Change `get_mut()` now acquires the write lock first and reloads the dataset while holding it, so write-fairness covers the whole operation: as soon as a writer calls `get_mut()`, new readers queue behind it. The reload body is factored into `reload_under_write_lock()`, which takes the already-held `RwLockWriteGuard`, probes `has_successor_version()` and calls `checkout_latest()` if needed. The read path (`get()` / `get_refreshed()` -> `reload()`) is unchanged in behaviour: it still checks under a read lock and only upgrades to a write lock when a successor version exists, then reuses the same helper instead of duplicating the probe-and-checkout logic. The cost is that every `get_mut()` now performs the successor probe while holding the write lock rather than under a read lock. Writers already had to take the write lock immediately afterwards, and `has_successor_version()` is a single object-store existence check on the manifest table, so the extra exclusive hold is short and only on the write path. ## Testing New unit test `test_get_mut_is_served_before_later_readers` in `rust/lance-namespace-impls/src/dir/manifest.rs`. It holds the wrapper's lock exclusively, starts `get_mut()` on one task and then, on a second task, a plain read-lock acquisition that keeps its permit until told to release it; it then releases the exclusive lock and asserts under a bounded timeout that `get_mut()` completes before the late reader is served, and that the reader completes once released. On a current-thread runtime the two tasks park on the lock in spawn order and the late reader holds a single permit from admission to release, so the outcome depends only on which lock `get_mut()` requests first: with the old ordering it queued as a reader, the late reader was admitted alongside it, and the writer's subsequent write request blocked on that reader until the timeout; with the new ordering it is queued as a writer and served first. Verified both ways, five runs each, with the fix locally reverted and restored. The rest of the `lance-namespace-impls` suite (which exercises `get()` / `get_refreshed()` through the directory namespace operations and `get_mut()` in one existing unit test) passes unchanged, along with `cargo fmt --all -- --check` and `cargo clippy --all --tests --benches -- -D warnings`. ## Compatibility Internal locking change only. No public API, wire, or file-format changes; the manifest dataset layout is untouched. ## Prior submission The same fix was previously submitted as #6323 and went stale unreviewed. --- **Tracking:** Ported from rerun-io#70. Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
amunra
added
upstreamed
Author
|
Rebase note. Upstream as lance-format/lance#9285 ( A release branch cut from an upstream commit at or after lance-format#9285 already has it: drop this commit rather than carrying it forward. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DatasetConsistencyWrapper::get_mutreloaded the dataset and then took the write lock, so the reload's read lock was released before the write lock was requested. Under a steady stream of readers the writer could keep losing that race. It now takes the write lock first and reloads while holding it, so tokio's write-fairness applies to the whole operation.The reload body is factored into
reload_under_write_lock, shared with the read path'sreload, which still checks under a read lock and upgrades only when a successor version exists.Lineage: authored by Emil as
d543ed8419eonemilk/fix-write-starvation(#12, closed unmerged), then landed by cherry-pick as #13 intorelease-3.0.0and #14 intorelease-2.0.0. It reached the current release lines as996affee5458af936d78352e80055cf16f4b25b6onrelease-8.0.0and1b47099f20c559f0022a5b8a42b995f6f256bc81onrelease-9.0.0, which are identical to each other and are what this ports, unchanged.The r8/r9 form is not identical to the original: the shape of the reload it wraps changed on the way, and this port matches r8/r9 rather than
d543ed8419e. #15, the follow-up to therelease-3.0.0cherry-pick, also touches this file but is unrelated to the locking — comment cleanup and error-construction style on the read path, against a version ofreloadthat usedlatest_version_idrather thanhas_successor_version.