Skip to content

Fix data race possible from safe code on ClusterPointInner - #38

Open
Ollie-Pearce wants to merge 1 commit into
serega:masterfrom
Ollie-Pearce:fix-data-race
Open

Ollie-Pearce wants to merge 1 commit into
serega:masterfrom
Ollie-Pearce:fix-data-race

Conversation

@Ollie-Pearce

Copy link
Copy Markdown

The ClusterPointInner type implements Send and Sync, allowing for instances to be shared across threads. The type contains a Cell field and methods on the type make unsynchronised accesses to this field.

#[derive(Debug, Clone)]
pub struct ClusterPointInner<Id> {
    pub id: Id,
    pub cluster: Cell<Option<usize>>
}

unsafe impl<Id> Send for ClusterPointInner<Id> {}
unsafe impl<Id> Sync for ClusterPointInner<Id> {}

Reproduction:

Test case:

#[test]
fn clusterpointinner_race() {
    let point: ClusterPointInner<bool> = ClusterPointInner::new(true);
    std::thread::scope(|s| {
        s.spawn(|| {
            let _ = point.clone();
        });
        point.assign_cluster(1);
    });
}

Verifying with miri:

cargo +nightly-2025-08-20 miri test -p gaoya --test repro

Output:

test clusterpointinner_race ... error: Undefined Behavior: Data race detected between (1) non-atomic write on thread `clusterpointinn` and (2) non-atomic read on thread `unnamed-2` at alloc40184
    |
546 |         unsafe { *self.value.get() }
    |                  ^^^^^^^^^^^^^^^^^ (2) just happened here
    |
help: and (1) occurred earlier here
    |
 31 |         self.cluster.set(Some(cluster_id));
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Fix:

  • Remove Send and Sync implementations from ClusterPointInner in clustering_serial.rs
  • If I understand correctly while the ClusterPointInner type in clustering_parallel.rs does need to be Send and Sync the trait implementations are not required for the equivalent type in clustering_serial.rs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant