Fix: Preserve Once state across moves

[lukenels]

sync::Once is a synchronization primitive to execute an initialization routine exactly once in the case of multiple threads. There's currently an issue in Shuttle's implementation of the Once primitive that gets triggered when a once is moved to a new location:

fn bar() {
    // Allocate a Once, initially is_completed = false
    let once = Box::new(Once::new());
    assert!(!once.is_completed());

    // Invoke call_once, now is_completed = true
    once.call_once(|| {});
    assert!(once.is_completed());

    // Move the Once to a new location, is_completed should still be true.
    // However, Shuttle's version resets back to is_completed = false
    // and this assertion fails.
    let once = Box::new(*once);
    assert!(once.is_completed());
}

The problem is that Shuttle's implementation of Once uses the address of the Once as a unique key to look up the Once's state in ExecutionState storage. But when the Once is moved, it gets a new address, and so it effectively becomes an entirely new object from Shuttle's perspective. For static uses of Once, this will never matter as the object lives for the duration of the program and cannot be moved from. But Onces need not be static, as shown above.

This commit introduces a new unit test case demonstrating the problem, and fixes the problem by adding a layer of indirection: instead of using the address of the Once itself as the storage key, the Once will (lazily) allocate a dummy Box<usize> and use the address of the allocation as the key. This fixes the problem because the inner address (and therefore, the storage key) is preserved even if the Once itself is moved.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[sarsko]

What am I missing? Why can't this just be a monotonically increasing usize (ie. dummy becomes id)?

[lukenels]

I considered that; I could do something like this:

fn id(&self) -> usize {
    static NEXT_ID: StdAtomicUsize = StdAtomicUsize::new(1);

    let id = *self.id.get_or_init(|| NEXT_ID.fetch_add(1, Ordering::Relaxed));
    assert_ne!(id, 0, "id overflow");
    id
}

The main reason I didn't is that I don't want to have to reason about the overflow case, using Box abuses the allocator to give a way to release identifiers when dropped. On platforms where usize is 64-bit, seems reasonable to assume it'll never overflow, but for 32-bit targets it seems possible.

[sarsko]

I'm not sure I share your worry; if someone is able to to allocate over 4 billion Onces in a single Shuttle test then I'll be mightily impressed.

If you really think this can happen, then surely an AtomicU64 would be suited.

[lukenels]

I would use AtomicU64 for the counter but StorageKey uses usize anyways so it wouldn't help. I agree it's unlikely to exhaust a the counter for 32-bit targets, but thought it's simpler to not have to make that argument at all. I'm happy to switch to the counter-based implementation though, I doubt there are many non-static Onces in the wild, and we'd have to run a lot of test case iterations in order to exhaust the id space.

[sarsko]

Yeah, I getcha. I feel like an id-solution is less "foreign" and feels less like a trick, which I value more than theoretical overflow possibilities on 32 bit architectures.

I wonder if it is worth it to change from a static counter to a per-execution counter (a way to do it would be to add a call to ExecutionState::cleanup where we reset static counters), but that might be more hassle than it is worth.

[sarsko]

Thanks for the PR btw, I think with the Clippy addressed and optionally the suggested addition to the test the PR looks good to me.