Make loom a dev-dependency (and upgrade proptest)

[djc]

I'm not sure this doesn't muck up the way you use loom, but it would be nice if loom was listed as a dev-dependency (as it is in tokio) so that its dependencies don't end up being in downstream Cargo.lock files.

(Upgraded proptest too since I happened to notice it and the upgrade seemed trivial anyway.)

[hawkw]

The reason loom isn't presently a dev dependency is that we rely on thread-local storage. When used with loom, loom's "threads" are not actually separate OS threads, so they all share the same value for thread-locals. This results in wrong behavior. Instead, we use loom's thread_local! macro, which simulates a thread-local.

The intention behind always using loom when cfg(loom) is set is so that we behave correctly in a dependent crate's loom tests. Otherwise, if an upstream crate writes a loom test for some code that depends on this crate, we will behave incorrectly, resulting in wrong behavior in the test.

I thought that it was important to not break downstream loom tests if they use APIs from this crate. But, if nobody's writing loom tests that use our APIs, I'd be willing to reconsider if people think not seeing loom in their lockfiles is more important.

[djc]

That makes some amount of sense, though I wonder how to square this with the fact that tokio and bytes both list loom as a dev-dependency. How is their usage different; is it unreasonable to expect all of these crates to use the same method of depending on loom?

I suppose a potential compromise might be to have loom also guarded by an off-by-default feature.

[hawkw]

I wonder how to square this with the fact that tokio and bytes both list loom as a dev-dependency. How is their usage different; is it unreasonable to expect all of these crates to use the same method of depending on loom?

I think these cases are somewhat different than this crate. In general, I think most crates using loom expect that they will not be part of dependent crates' loom tests, and that they will essentially be treated as a black box by simulations of their dependents, relying on the crate's own crates' loom tests to ensure that its behavior is correct.

This approach makes sense for tokio and bytes. bytes only uses loom's simulated AtomicPtr, and a loom simulation that includes code using std::sync::AtomicPtr will behave correctly; it just doesn't include that AtomicPtr in the model. Essentially, because loom simulates multithreaded behavior in a single real thread, all std atomics in a loom simulation are essentially treated as always being sequentially consistent, since they are never actually accessed across threads. This means that if dependencies are treated as black boxes by loom tests, the dependency still behaves as expected, but is assumed to be correct.

However, some of the std APIs that loom simulates don't behave the same as the atomics. Thread locals are a primary example of this: a std::thread_local! will return the same value for every loom::thread in a loom model. In some cases this results in wrong behavior. bytes does not use loom's simulated thread locals, so it doesn't have to worry about this. For this crate, on the other hand, behavior is dependent on thread IDs that we generate using thread locals. Code that behaves one way in "real life" might behave differently in a loom test, which is not really the case for bytes.

I suppose a potential compromise might be to have loom also guarded by an off-by-default feature.

I'd be happy to do that. If the feature flag is enabled, we depend on loom for all cfg(loom) builds; otherwise, it's a dev-dependency. That way, users who actually use this crate's APIs in a loom test can enable loom in their tests, but it doesn't show up in anyone else's lockfiles.

[djc]

👍 Implemented a way to do it as an optional dependency. Let me know if you agree this is an appropriate setup. The duplication across dependencies and dev-dependencies is a little unfortunate, but AFAICT there is no easy way around it.

[djc]

Ah sorry, you're right of course. Rebased to squash your suggestions into my original commit.

[djc]

I hadn't yet -- not sure why CI doesn't run for this PR? In any case, I've run it now, and it works after fixing one more typo.