sync feature seems to have caveats not mentioned in the docs

[ejrinkus]

I ran into an issue today while enabling the sync feature for some tests.

One of my tests is intended to verify that our concurrency logic WAI, and then we have a few other tests just checking some error cases.

Since we needed to enable sync for the concurrent test, that means that it's enabled for all the tests. This caused the tests to clobber each other because they all seem to be using the same MockClock singleton when that feature is enabled. The solution ended up being to serialize all the tests with the serialize_test crate so that each test could reset the clock when it started and not have anything else clobber it.

The docs don't seem to call out this caveat anywhere when mentioning this feature, nor is there any prescribed way to handle these situations if a different method is preferred.

Could we get this added to the docs, or prescribe another workaround if there's a better one? Understandable that this is tough to work around given the API that this lib is mocking, so I think making this clear in the docs is sufficient.

[museun]

This is a problem with how cargo combines features. A workaround, that I can think of, is to remove the automatic lazy-initialization of the clock and have the user initialize it per-thread/per-executable.

Adding a cavaet to the docs explaining how feature feature coalescing works would be a first step.

This is an explanation of what cargo is doing. https://doc.rust-lang.org/cargo/reference/features.html?highlight=additive#feature-unification.

[ejrinkus]

Thanks appreciate you calling this out in the docs, this should at least help surface the issue for other people with similar use cases to mine!

As for solutions, the way that Rust handles time I think makes this a bit tricky. In the past, I've seen other approaches where an actual clock object is passed/shared with code that needs to track time. This object can then be mocked directly, rather than mocking bare functions via dependency injection.

It's a nice approach for libraries, because it means that the lib can offload concurrency handling to users; if you need to access the clock concurrently, then wrap it in an Arc<Mutex<>> and share that across threads. It also means individual tests running concurrently each have a separate clock instance.

I'm not sure how this would work in Rust though, unfortunately, given that time functionality is provided via functions instead of objects. It would likely mean providing a struct that wraps std time functions in a mockable object instead, which seems a bit orthoganal to the goals of this project.

Only other suggestion I have is seeing if it's possible to make sync a runtime option rather than a feature flag, but I'm not familiar with this codebase and so I don't know how feasible that is.

At the end of the day, synchronizing tests was an acceptable workaround for us, and so recommending that for use cases like mine might just be the best solution for now.