SharedPtr potential race condition fix - #466
Conversation
vporoshok
left a comment
There was a problem hiding this comment.
Thanks — the ordering rework is the right shape, and it quietly fixes more than the title suggests. A few things before this goes in.
Please expand the PR description. It is empty right now, and this is not only a race-condition fix: it also corrects a memcpy byte count that silently under-copied data, repairs an ill-formed self-assignment guard in SharedSpan, and removes a double construction in push_back. Those deserve to be written down — the commit outlives our memory of this review, and the title ("SharedPtr potential race condition fix") does not even mention GenericVector, where nearly the whole change lives, while SharedPtr itself is untouched.
The reordering alone does not make this race-free. See the inline note on resize: the item count is a plain non-atomic field, so concurrent readers stay in UB territory and the compiler is free to move the size store across the element stores. The publication needs an atomic store/load pair to mean anything.
No tests. Two of the three bugs fixed here are trivially testable, and one of them — the memcpy length — would have been caught long ago by a single test with sizeof(T) > 1. Please add coverage; details inline.
vporoshok
left a comment
There was a problem hiding this comment.
Reviewed the whole diff. The core of the change looks right to me:
- the
set_sizereorderings inGenericVectorare semantically equivalent to the previous branching for every trait combination I checked (IsZeroInitializable/IsTriviallyDestructible/ trivial); - dropping
reserve()on the shrink path is harmless —grow_to_fit_at_leastis a no-op when shrinking; - the
size * sizeof(T)memcpy fix and thethis != &otherself-assignment fix are genuine bugs fixed; - all instantiation sites of the new
ControlBlockTypetemplate parameter were updated consistently; - the atomic control block keeps the same size and layout (8 bytes), so no serialized format changes.
Two inline comments below. Plus one finding in a file outside this diff:
pp/primitives/snug_composites_filaments.h — Symbol::emplace_back publishes before writing the data. It appends the items_ entry (pos/length) first and only then appends the bytes to data_. A reader that observes the newly published item count can therefore read bytes that have not been written yet, or read past the end of data_. The vector-level ordering fix in this PR is not observable through this layer until the order here is swapped: bytes into data_ first, then publish the items_ entry.
Two things I initially flagged and am explicitly not asking to change here:
pp/series_index/reverse_index.hstill uses the non-atomic counter — agreed this belongs in a separate PR; the snapshot copies the encoder as a whole and only reads the element count captured at snapshot time, and since data is append-only that does not misbehave.GenericVector::erase()publishes the smaller size and then memmoves the tail into the still-published range, so the reorder does not makeerasesafe for concurrent readers — buteraseis not used with shared memory and cannot be made safe anyway, so this is an understood precondition.
…ptr_race_condition_fix
…ptr_race_condition_fix
…ptr_race_condition_fix
vporoshok
left a comment
There was a problem hiding this comment.
All three findings from my earlier review are addressed:
push_backnow usesstd::uninitialized_copy, with aVector<Vector<uint32_t>>test that exercises exactly the non-trivially-copyable fallback branch.- The item counter uses explicit
load(acquire)/store(release)instead of the seq_cst defaults. Symbol::emplace_backcapturesdata_sizeup front, appends the bytes, and only then publishes theitems_entry.
The sentinel refactor on top of that is a good addition: end() becomes a static sentinel, operator==(IteratorSentinelType) no longer dereferences storage_ptr_ (which also removes an uninitialized-pointer read for the default-constructed iterators in LabelSet::get_values_range), and sentinel_id_ trims the tail that can appear because a read-only span reads the live items_count from the shared control block while the writer keeps appending.
I raised four more points offline; all four were answered and none of them stand:
- the
sentinel_id_mechanism needs no comment or caching — a single id-based iterator is exposed andbegin()is called once; apos + lengthoverflow pastuint32_tmeans the data is already lost either way; operator[]is unvalidated by design and ids cannot be obtained unless they were handed out;size()is an upper bound used for reservation, which is fine;- snapshots are taken under a lock, so there is no window in the read-only ctor;
- the non-atomic refcount fast paths rest on the existing single-owner + Go-level-lock convention, untouched by this PR.
CI is green on both architectures, including go-test-pp under ASan. LGTM.
* changed order of set_size in vector append operations * changed order of set_size in vector erase operations * review fixes * created AtomicSharedPtrControlBlockWithItemCount and used in entrypoint QEB * review fixes * fixed Vector::push_back bug for NonTriviallyCopyable objects * changed memory model for AtomicSharedPtrControlBlockWithItemCount to acquire/release * added removed static_assert * changed order of adding to items_ and data_ containers * added symbol validation in symbol iterator * refactoring * used BareBones::iterator::kSentinel instead concrete symbol iterator
Motivation
SharedVector's size lives in the shared-pointer control block (items_count) and can be read from another thread while the owning thread mutates the vector. Two problems made this unsafe:
Changes
• Growth (resize, push_back, insert, emplace_back, ranged push_back): construct/fill the new elements first, then publish the larger size.
• Shrink / erase / clear: publish the smaller size first, then destroy elements / memmove the tail.
• Split resize into grow_storage() / decrease_storage() helpers to make the two paths explicit and remove the previous nested branching.
• resize(new_size, value) now uses std::uninitialized_fill on the grow path instead of resize() + std::fill.
• Ranged push_back: std::memcpy copied size bytes instead of size * sizeof(T) — fixed.
• SharedSpan::operator=(&&): self-assignment check compared this != other (pointer vs object); fixed to this != &other.
• Introduced GenericSharedPtrControlBlockWithItemCount.
• SharedPtrControlBlockWithItemCount = the existing uint32_t variant (unchanged behavior).
• Added AtomicSharedPtrControlBlockWithItemCount = std::atomic<uint32_t> variant (default seq_cst load/store, giving the acquire/release publication needed by readers).
• SharedMemory / SharedVector / SharedSpan are now parameterized by the control-block type.
• Entrypoint LSS/QEB types (entrypoint/types/lss.h) now use AtomicSharedPtrControlBlockWithItemCount.
• reverse_index.h explicitly keeps the non-atomic control block (single-threaded usage).