Repository navigation
fix(once_map): clone only initialized entries in get - #387
Merged
Merged
Conversation
A get that cloned a pending entry kept the cleanup of a failed, panicked, or cancelled computation from removing it, so the empty entry stayed in the map. get returns None for a pending entry anyway, so it now clones only initialized entries, which are never reclaimed.
tisonkun
reviewed
Oct 25, 2026
Comment on lines
+135
to
+136
| #[cfg(test)] | ||
| tests::after_get_lookup(); |
Member
There was a problem hiding this comment.
Why do we have this #[cfg(test)] sentence?
Can we get rid of it in some way?
Contributor
Author
There was a problem hiding this comment.
It let the test pause get right after its lookup so the computation could be cancelled in that window.
I thought about it again, and that wasn’t a good approach. The test can hold find_entry’s result across the cancellation on one thread instead, so I removed the hook. Since that is less obvious than a paused get, I added a comment saying it stands in for a concurrent get.
The regression test now holds find_entry's result across the cancellation on one thread, which is the window get used to keep the entry alive in, so get needs no test-only code.
This was referenced Oct 26, 2026
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.
Summary
OnceMap::getclones the entry'sArcunder the table lock and reads the cell after unlocking. If the key is still being computed and that computation fails, panics, or is cancelled inside this window, its cleanup sees three owners (the table, itself, and the reader) and leaves the entry in place. The reader then returnsNoneand drops its clone without cleaning up, so the empty entry stays in the map until the same key is computed or discarded again. With distinct keys these entries pile up: locally, four threads callinggeton the key under computation while 20,000try_computecalls failed left about 9,000 to 9,800 empty entries.gethas no use for a pending entry, since it returnsNonefor it anyway, so this change only clones entries that are already initialized. Initialized entries are never reclaimed, so a reader holding one cannot interfere with cleanup, and pending lookups also skip theArcclone and drop.A new unit test cancels a computation while holding the result of the lookup
getperforms, which is the window wheregetused to keep the entry alive, and checks that no entry is left. It fails on the previous implementation with one entry left. The faulty behavior is in v0.7.4, so the fix has a changelog entry.Validation:
cargo x test(612 passed),cargo x check(26 feature configurations),cargo x lint, and the once_map unit tests under Miri.