Skip to content

feat: add a strategy to improve shuttle compatibility#171

Open
bdonlan wants to merge 2 commits intovorner:masterfrom
bdonlan:shuttle
Open

feat: add a strategy to improve shuttle compatibility#171
bdonlan wants to merge 2 commits intovorner:masterfrom
bdonlan:shuttle

Conversation

@bdonlan
Copy link

@bdonlan bdonlan commented Sep 11, 2025

Currently, ArcSwap does not integrate with the shuttle concurrency testing tool. While this doesn't break code using shuttle, it also does not test for conditions such as retrying an rcu update. This change exposes a ShuttleStrategy based on the internal RwLock<()> strategy, which uses shuttle's RwLock primitive. This then allows shuttle to reorder operations on ArcSwap and expose races caused by these operations.

This needs to be implemented in ArcSwap itself (rather than implementing it in shuttle) as the necessary traits are currently sealed.

Currently, ArcSwap does not integrate with the [`shuttle`] concurrency testing tool.
While this doesn't _break_ code using shuttle, it also does not test for conditions
such as retrying an `rcu` update. This change exposes a `ShuttleStrategy` based on the
internal `RwLock<()>` strategy, which uses shuttle's `RwLock` primitive. This then allows
shuttle to reorder operations on ArcSwap and expose races caused by these operations.

[`shuttle`]: https://github.com/awslabs/shuttle
Copy link
Owner

@vorner vorner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello

I have few issues here:

  • Shuttle is pre-1.0 crate. arc-swap is 1.0. Therefore, it shouldn't really have a dependency this way. I think it would make sense to include shuttle as part of the internal-test-strategies, which is clearly marked as not stable (I'm fine rewording it to be less scary).
  • What are the plans of support / migration when new version of shuttle comes out?
  • The shuttle strategy seems to be mostly a copy paste of the rw_lock one. I don't like the duplicate code. Also, why do you also enable the rw_lock module in case you enable the shuttle one?

@bdonlan
Copy link
Author

bdonlan commented Sep 16, 2025

  1. I'm happy to put this under the internal-test-strategies feature flag if you prefer, - though, perhaps an unstable-shuttle flag would be better to avoid pulling in shuttle as a dependency for the internal-test-strategies FF. Ideally we'd be able to just implement this in an external crate, but I guess the arcswap internal traits aren't ready for stabilization yet?
  2. We depend here only on the shuttle synchronization primitive shims, which are unlikely to change (they need to match the std::sync API in any case). In the unlikely event that this does change, I suppose it would need to be a breaking change (within the internal-test-strategies feature flag?)
  3. The way shuttle works is by providing an API-compatible version of the std::sync synchronization primitives. Unfortunately, those primitives do not expose their API through a trait, which makes it difficult to avoid repeating the implementation here. I suppose I could make it a macro, if you prefer it that way. As for why I enable the rw_lock module, it's because the impl<T: RefCnt> Protected<T> for T implementation is needed for both; I could move that up to strategy/mod.rs if you'd prefer.

@bdonlan
Copy link
Author

bdonlan commented Sep 16, 2025

Technically, we also depend on other aspects of the shttle API for our unit tests, but those would not be a breaking change to adjust, if the shuttle API were to change in the future.

@bdonlan
Copy link
Author

bdonlan commented Sep 23, 2025

@vorner Any further concerns on this one?

/// [`shuttle`]: https://github.com/awslabs/shuttle
/// [`rcu`]: crate::ArcSwapAny::rcu
#[derive(Default)]
pub struct ShuttleStrategy {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this wrapper serves an actual purpose. The real meat seems to be in the other module.

@@ -0,0 +1,120 @@
use shuttle::sync::RwLock;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say the feature or module would deserve some documentation about what it is for and that it is considered unstable. I know the comment is somewhere within Cargo.toml, but that one isn't rendered at docs.rs and people usually don't read that one.

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.

2 participants