0.2.1 (current) Thoroughness: High Understanding: Medium
by yvt on 2021-09-20
This review is from Crev, a distributed system for code reviews. To add your review, set up cargo-crev
.
0.2.1 (current) Thoroughness: High Understanding: Medium
by yvt on 2021-09-20
Crates in the crates.io registry are tarball snapshots uploaded by crates' publishers. The registry is not using crates' git repositories. There is absolutely no guarantee that the repository URL declared by the crate belongs to the crate, or that the code in the repository is the code inside the published tarball.
To review the actual code of the crate, it's best to use cargo crev open atomic-ref2
. Alternatively, you can download the tarball of atomic-ref2 v0.2.1 or view the source online.
This crate provides an atomic cell type to store
Option<Arc<T>>
.Major issues
The bounds for
Atomic[Option]Ref<T>: Send + Sync
are unconstrained, which allows thread-unsafe objects to be sent between threads, potentially leading to data race.Minor issues
The atomic operations in this crate are riddled with seemingly unnecessary
Ordering::SeqCst
, which is considered by some (including myself) to be a code smell.While most atomic operations in this crate are
SeqCst
, there are a few places whereRelaxed
is used. Although stress-testing did not reveal any problems, there's a theoretical soundness issue regarding the usage ofRelaxed
as explained in the next section. It's very possible that the excessive usage ofSeqCst
is why this issue doesn't seem to surface in practice.Theoretical issue with the
Relaxed
memory orderingDisclaimer: I'm not expert in the C++20 memory model, and therefore the assessment presented here might be incorrect.
Shown below is a simplified code listing of
AtomicOptionRef
's each method with a syntax loosely inspired by Promela. Brackets indicate the memory orderings for atomic operations ([SC]
=SeqCst
,[]
=Relaxed
).Suppose two threads execute the following sequences (multiple execution instances of a single program line are distinguished by alphabetic suffixes):
Also suppose that T2 observed the write performed by 23b at 10. The question is, which
self.ptr
will T2 observe at 11? It would be unsafe to observe theself.ptr
written by 22a because thatself.ptr
is returned to the caller by 22b, and by the time 12 is executed, it might refer to an already-deallocated memory region. For this unsafe case to manifest, the total orderS = [20a, 22a, 20b, 10, 11, 22b]
onSeqCst
operations must be valid.For this total order to be valid, it must be consistent with the "strongly happen-before" relationship (C++ N4861 31.4 4) and the "coherence-ordered relationship" on every object (C++ N4861 31.4 4.1). It's clear from the above diagram that
S
meets this requirement.Furthermore, for 11 to observe the write by 22a, 22b must not happen-before 11, which is also met as shown in the above diagram.
Therefore, it's possible for T2 to observe the write by 22a at 11, meaning, during this method call,
AtomicOptionRef::load()
can cloneArc<T>
that has been previously returned byAtomicOptionRef::swap()
, which can cause an undefined behavior because it's possible that thisArc<T>
is dropped by the caller, and its control block doesn't exist anymore.Meta Issues
The
homepage
field of the package metadata is incorrect. The correct URL is https://github.com/mehcode/atomic-ref2-rs.Issue: High (github.com/mehcode/atomic-ref2-rs/issues/1)
Atomic[Option]Ref<T>
implementsSend + Sync
unconditionally