meck:unload/0 sometimes crashes

[legoscia]

Every now and then, meck:unload/0 crashes with not_mocked for me. It seems to be a race condition: unload/0 passes everything in registered() that locks like a mock to unload_if_mocked, but if the mock module has already been unloaded at that point (due to having been mocked by a now dead process), a not_mocked error is raised and the remaining modules are not unmocked.

I could reproduce it with this Proper property (because it runs the test enough times to make the race condition more likely):

-module(foo).

-compile(export_all).
-include_lib("proper/include/proper.hrl").

prop_unload() ->
    ?FORALL({Wait1, Wait2}, {nat(), nat()},
            begin
                {_, Ref} = spawn_monitor(fun() ->
                                                 timer:sleep(Wait1),
                                                 meck:new(foobar)
                                         end),
                timer:sleep(Wait1 + Wait2),
                meck:unload(),
                receive {'DOWN', Ref, process, _Pid, _Reason} -> ok end,
                true
            end).

Failure looks like this:

22> proper:quickcheck(foo:prop_unload()).
..
=ERROR REPORT==== 4-Nov-2011::14:16:36 ===
Error in process <0.1056.0> with exit value: {{already_started,<0.1054.0>},[{meck,new,[foobar,[]]}]}

......!
Failed: After 9 test(s).
An exception was raised: error:{not_mocked,foobar}.
Stacktrace: [{meck,gen_server,3},
             {meck,unload,1},
             {meck,unload_if_mocked,2},
             {lists,foldl,3},
             {foo,'-prop_unload/0-fun-1-',1}].
{4,4}

Shrinking .(1 time(s))
{1,4}
false

This patch seems to fix the problem:

diff --git a/meck/src/meck.erl b/meck/src/meck.erl
--- a/meck/src/meck.erl
+++ b/meck/src/meck.erl
@@ -400,7 +400,8 @@ unload_if_mocked(P, L) when length(P) > 5 ->
     case lists:split(length(P) - 5, P) of
         {Name, "_meck"} ->
             Mocked = list_to_existing_atom(Name),
-            unload(Mocked),
+            catch call(Mocked, stop),
+            wait_for_exit(Mocked),
             [Mocked|L];
         _Else ->
             L

[eproxus]

The fix should probably be try call(Mocked, stop) catch exit:{not_mocked, Mocked} -> ok end?

[legoscia]

Right, that's a nicer fix (except that it should be error instead of exit ☺). I've now convinced myself that this is the best way to do it:

diff --git a/meck/src/meck.erl b/meck/src/meck.erl
--- a/meck/src/meck.erl
+++ b/meck/src/meck.erl
@@ -400,7 +400,11 @@ unload_if_mocked(P, L) when length(P) > 5 ->
     case lists:split(length(P) - 5, P) of
         {Name, "_meck"} ->
             Mocked = list_to_existing_atom(Name),
-            unload(Mocked),
+            try
+                unload(Mocked)
+            catch error:{not_mocked, Mocked} ->
+                    ok
+            end,
             [Mocked|L];
         _Else ->
             L

This also avoids monitoring the dead process.

[eproxus]

Didn't add any proper property for this yet, so unfortunately there's no coverage for 4th added line in the patch.