Mocks panic when accessed from multiple threads

[Wesmania]

On 0.0.8, this minimal example panics:

#[faux::create]
struct Foo { }

#[faux::methods]
impl Foo {
    pub fn foo(&self) {}
}

fn main() {
    let mut fake = Foo::faux();
    faux::when!(fake.foo).then(|()| {});
    let fa1 = std::sync::Arc::new(fake);
    let fa2 = fa1.clone();
    let t1 = std::thread::spawn(move || loop { fa1.foo() });
    let t2 = std::thread::spawn(move || loop { fa2.foo() });
    t1.join().unwrap();
    t2.join().unwrap();
}

try_lock() in morphed.rs seems to be the cause.

[nrxus]

Hm, this is kind of tricky.

faux stores the mocks in a Mutex<MockStore>. In order to access them, we need to lock the mutex. I originally did try_lock instead of lock thinking:

At least it will panic instead of deadlocking

But looking at your example, doing a lock should work. I am still concerned there may be some edge case out there where doing lock would case a deadlock, but maybe having it work for the easy case is worth that.

What do you think?

[Wesmania]

One issue I can think of is that a global lock can cause a deadlock when calling different methods. Something like that:

faux::when!(foo.baz).then(|()| { a.lock(); a.unlock(); });
fn thread1() {
    a.lock();
    foo.bar();
    a.unlock();
}

fn thread2() {
    foo.baz():
}

First thread1 locks a, then thread2 calls baz and waits, then thread1 calls bar and we deadlock.

One way to alleviate that could be to wrap each mocked method in an Arc<Mutex<_>> so we can clone the method out of MockStore and release its lock immediately. That protects us from most surprises.

[nrxus]

I don't understand how an Arc<Mutex<_>> would protect us from a deadlock, can you please elaborate a little more?

For what it's worth, .then can only be passed in a 'static closure, so the only way it could borrow a lock (like it does in your example) is through then_unchecked which is marked as unsafe. This should already hint users to be very careful.

[Wesmania]

From what I understand, the MockStore lock is taken for the entire duration of a call, so it synchronizes access to all mock's methods, causing an unexpected dealock described above. The idea behind storing an Arc<Mutex<_>> is to grab the MockStore mutex only to clone the Arc out, then hold the method's mutex during the call. It doesn't eliminate deadlocks entirely, but it no longer locks the entire object, so playing around with sleeping/locking inside a mock method is less likely to cause unexpected deadlocks.

[nrxus]

Ohh I see, that's an interesting thought. That gets a little tricky as it needs to be wrapped in a Mutex<> for the entire mock store already as some mocks get consumed. I think for now I will change it to be a lock rather than try_lock and add a warning on the docs somewhere regarding possible deadlocks. This is a great catch, thanks!

And of course I welcome PRs if you beat me to the fix since I have been very focused on the new api changes lately, and I still have work to do there.

[Wesmania]

I think https://github.com/nrxus/faux/pull/36 is a good start, though I think I completely ignored the mock consuming bit.