Static methods & mutexes idea/question

[jder]

Hi all! I'm experimenting with using mockall for mocking out a number of modules with static methods. It generally works great, but I've run into two papercuts:

First, it's pretty easy to accidentally write something like:

my_mod::some_static_function_context().expect().times(1);

Which doesn't do what you want since the context object is immediately dropped and panics. It's harder to do this for non-static mocks since the context object is also the mock object (and so you'll need to keep it alive for longer).

Second, you have to remember to take some kind of mutex in each test (as the docs kindly call out). In practice for us this means we'll need one process-wide mutex or have to be very careful about the order of mutexes we take for different module mocks in order to prevent deadlocks. Sounds like there's been some discussion about the risks of trying to solve this (would love pointers to them if anyone has them handy).

I was wondering if we could solve both of these by allowing test code to look something like this instead:

let context = my_mod::mockall::lock_context();
context.expect_some_static_function().times(1);
…

Basically, more closely mimic the non-static API. The lock_context function could take a process-wide mutex on your behalf and hold until the context object is dropped. You could still have the first bug I mentioned but it's (a) less likely because you'll often set multiple expects on the same context object and (b) the "lock" name of the method might make you think of mutex guards which you need to hold on to (and might also hint that you should think about deadlocks with any of your own mutexes).

Would that be something you'd like to see in mockall? I'd love to discuss contributing it, perhaps under an attribute argument like #[automock(static_lock)] if you're interested. Thanks!

[asomers]

I can think of two problems that would arise pretty quickly:

• You described lock_context as process-wide, but it's under the my_mod namespace. Does that mean that if you're mocking multiple modules or types, you'd have to grab the process-wide lock twice? Would it be a recursive Mutex then?
• If not, how would you name the expectation functions? my_mod needs to be in there somewhere.
• Using a process-wide mutex would greatly limit the test suite's concurrency. Unnecessarily, IMHO. Using fine-grained mutexes, one for every mocked static function, would result in better performance.

BTW, it would be easy to build the Mutex into the Context object. The only reason I didn't is because it would be easy for users to accidentally trigger deadlocks, because they don't know about the Mutex's existence. If somebody ever writes a Mutex type for Rust that has builtin lock-order-reversal detection, then we could use that.

[jder]

Thanks for the quick response! Yes, I was imagining a recursive mutex.

For my purposes, I think a process-wide mutex would be acceptable, but one other option would be to allow the user to declare which mutex they'd like to use eg#[automock(static_lock(MY_LOCK))] so they could do more fine-grained locking. What do you think?

[asomers]

If the users have to declare their own Mutex names, then I see little benefit over the status quo.

[jder]

To me it seems beneficial because you can't forget to take the mutex in your test. (I would also suggest letting it default to a process-wide lock as described above so you don't have to declare your own mutex unless you want to increase parallelism.)

[jder]

Looks like someone has built deadlock-detecting mutexes on top of parking_lot: https://github.com/bertptrs/tracing-mutex

[asomers]

Good find! I've been hoping that something like that would come along. The freeBSD kernel has the same mechanism, and it's very helpful. https://man.freebsd.org/cgi/man.cgi?query=witness .

[asomers]

@jder it looks like Clippy has the ability to add crate-specific lints. See this thread for more info: rust-lang/rust-clippy#6872 . Do you think it would be possible to add a lint to guard against the immediately-dropping-a-context-object bug?

[jder]

Yeah, I like this idea if I can't sell you on the other proposal 😀

[asomers]

Here it is: rust-marker/design#49 .