Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

File bif passthrough #36

Closed
wants to merge 4 commits into from

4 participants

@ghost

A deadlock occurs if meck is used to mock a module that is needed to recompile modules. The rough sequence of events causing the deadlock is the following when the file module is mocked:

1.  meck successfully loads a replacement for the file module ([unstick,passthrough])
2. add an expectation to the file module using meck:expect
3. a call is made to the file_meck server
4. the file_meck server updates the list of expecations
5. the file_meck server recompiles the replacement of the file module
5.1. during recompilation the previously loaded replacement module is used
5.2 all calls to the replacement module are redirected to the file_meck server
6. during recompilation a call is made to the file:get_cwd() function by a compiler worker.
7.  deadlock.

I resolved the issue by making a worker process responsible for executing recompliation and notifying the client when the module has been reloaded. In this case it enables the file_meck server to respond to calls
during this time.

@eproxus
Owner

I've also been aware some time that mocking the modules involved with compilation and code loading is troublesome, to say the least. :-) Great summary of what goes wrong.

I won't pull this in now for various reasons, the biggest being that I'm planning to (sometime) rewrite the code loading not to use the code module (instead using erlang:load_module/2, replicating some of the code module's features). This will most likely be a generic function in the meck_mod module.

@ghost

I had no expectations that this to be merged in without any adjustments on my end. It's good to know that you've already got an alternative solution in mind. Feel free to close this pull request and track the issue elsewhere.

@eproxus
Owner

Would this be an acceptable test case for this use case?

passthrough_file_test_() ->
     ok = meck:new(file, [passthrough, unstick]),
     ok = meck:expect(file, new_func, 0, ok),
     ?assertEqual(ok, file:new_func()),
     ?assert(meck:validate(file)),
     ok = meck:unload(file).

I can trigger the deadlock calling basically any function in any module used by any module that meck is calling during reloading code (thus being inside the meck gen_server). The file module is the most obvious example.

@ghost

I ran this test against the master where it failed and the file-bif-passthrough branch where it passed, only thing i would change is to remove the last underscore to confuse eunit less.

@eproxus
Owner

Ah, yes of course. ;-)

@samuelrivas

Hi, what is the status of this issue? I can still feel the pain with latest master version. Is there any work to be done that I could help with to fix this issue?

Mocking file:read_file will simplify some tests I want to write in a great deal ...

@ghost

How ironic, resurrecting an issue from the dead during the Easter holidays.

+1 I'm also wondering what the status of this issue is. Never know when I'll have to mock the file module again.

@eproxus
Owner

I'll pull down this branch and test it again. Already now I can say that I'd like the compilation state (running / not running) inside the gen_server state instead of in the process dictionary...

@eproxus
Owner

@klaar I've rebased your branch onto master here: https://github.com/eproxus/meck/tree/support/klaar/file-bif-passthrough so you can get a feel for how the changes would merge onto the current master.

Unfortunately the /proc filesystem does not exist on OS X so the test fails. Is it possible to rewrite it to run locally inside the test folder so that it's portable? Can you rebase your branch onto my master again?

@eproxus
Owner

Any progress on this?

@samuelrivas

I hope you are not expecting anything from my side :)

@DBarney

I just ran into this as well. +1

@horkhe

@eproxus I would like to take these changes, rebase them, and adjust the unit tests as per your comment. But I am not sure if I can add my commits to this pull request, so should I just create another pull request or what?

@eproxus
Owner

Please create a new pull request, this is way too far behind in commits. See if you can include the original commits so all authors can be attributed, otherwise include them in the commit message perhaps?

@horkhe

@eproxus ok, will do

@horkhe

You can peek at the result in my private repo under feature/file-bif-passthrough branch. It is base on the stub-all pull request and I will create a pull request for it as soon as the base is accepted.

@horkhe

@eproxus now we can close this saying that it was fixed by pull request #82

@eproxus
Owner

@horkhe Ok, didn't see that this was merged together with the refactoring branch.

@eproxus eproxus closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 11, 2011
  1. Add test for meck:expect on file module

    Magnus Klaar authored
  2. use a worker process to recompile modules

    Magnus Klaar authored
  3. crash on concurrent reload of mocked module

    Magnus Klaar authored
  4. don't log calls made during recompilation

    Magnus Klaar authored
This page is out of date. Refresh to see the latest.
Showing with 58 additions and 23 deletions.
  1. +40 −23 src/meck.erl
  2. +18 −0 test/meck_tests.erl
View
63 src/meck.erl
@@ -303,24 +303,24 @@ init([Mod, Options]) ->
handle_call({get_expect, Func, Arity}, _From, S) ->
{Expect, NewExpects} = get_expect(S#state.expects, Func, Arity),
{reply, Expect, S#state{expects = NewExpects}};
-handle_call({expect, Func, Expect}, _From, S) ->
- NewExpects = store_expect(S#state.mod, Func, Expect, S#state.expects),
- {reply, ok, S#state{expects = NewExpects}};
-handle_call({expect, Func, Arity, Result}, _From, S) ->
+handle_call({expect, Func, Expect}, From, S) ->
+ NewExpects = store_expect(S#state.mod, Func, Expect, S#state.expects, From),
+ {noreply, S#state{expects = NewExpects}};
+handle_call({expect, Func, Arity, Result}, From, S) ->
NewExpects = store_expect(S#state.mod, Func, {anon, Arity, Result},
- S#state.expects),
- {reply, ok, S#state{expects = NewExpects}};
-handle_call({sequence, Func, Arity, Sequence}, _From, S) ->
+ S#state.expects, From),
+ {noreply, S#state{expects = NewExpects}};
+handle_call({sequence, Func, Arity, Sequence}, From, S) ->
NewExpects = store_expect(S#state.mod, Func, {sequence, Arity, Sequence},
- S#state.expects),
- {reply, ok, S#state{expects = NewExpects}};
-handle_call({loop, Func, Arity, Loop}, _From, S) ->
+ S#state.expects, From),
+ {noreply, S#state{expects = NewExpects}};
+handle_call({loop, Func, Arity, Loop}, From, S) ->
NewExpects = store_expect(S#state.mod, Func, {loop, Arity, Loop, Loop},
- S#state.expects),
- {reply, ok, S#state{expects = NewExpects}};
-handle_call({delete, Func, Arity}, _From, S) ->
- NewExpects = delete_expect(S#state.mod, Func, Arity, S#state.expects),
- {reply, ok, S#state{expects = NewExpects}};
+ S#state.expects, From),
+ {noreply, S#state{expects = NewExpects}};
+handle_call({delete, Func, Arity}, From, S) ->
+ NewExpects = delete_expect(S#state.mod, Func, Arity, S#state.expects, From),
+ {noreply, S#state{expects = NewExpects}};
handle_call(history, _From, S) ->
{reply, lists:reverse(S#state.history), S};
handle_call(invalidate, _From, S) ->
@@ -332,12 +332,22 @@ handle_call(stop, _From, S) ->
%% @hidden
handle_cast({add_history, Item}, S) ->
- {noreply, S#state{history = [Item| S#state.history]}};
+ NewHistory = case get(meck_reloading) of
+ undefined -> [Item|S#state.history];
+ _Pid when is_pid(_Pid) -> S#state.history
+ end,
+ {noreply, S#state{history=NewHistory}};
handle_cast(_Msg, S) ->
{noreply, S}.
%% @hidden
-handle_info(_Info, S) -> {noreply, S}.
+handle_info({'EXIT', Pid, _Reason}, S) ->
+ %% XXX: don't do this
+ Running = get(meck_reloading),
+ Pid =:= Running andalso put(meck_reloading, undefined),
+ {noreply, S};
+handle_info(_Info, S) ->
+ {noreply, S}.
%% @hidden
terminate(_Reason, #state{mod = Mod, original = OriginalState, was_sticky = WasSticky}) ->
@@ -442,15 +452,22 @@ get_expect(Expects, Func, Arity) ->
{Other, Expects}
end.
-store_expect(Mod, Func, Expect, Expects) ->
- change_expects(fun e_store/3, Mod, Func, Expect, Expects).
+store_expect(Mod, Func, Expect, Expects, From) ->
+ change_expects(fun e_store/3, Mod, Func, Expect, Expects, From).
-delete_expect(Mod, Func, Arity, Expects) ->
- change_expects(fun e_delete/3, Mod, Func, Arity, Expects).
+delete_expect(Mod, Func, Arity, Expects, From) ->
+ change_expects(fun e_delete/3, Mod, Func, Arity, Expects, From).
-change_expects(Op, Mod, Func, Value, Expects) ->
+change_expects(Op, Mod, Func, Value, Expects, From) ->
NewExpects = Op(Expects, Func, Value),
- meck_mod:compile_and_load_forms(to_forms(Mod, NewExpects)),
+ %% If the recompilation is made by the server that executes a module
+ %% no module that is called from meck_mod:compile_and_load_forms/2
+ %% can be mocked by meck.
+ Pid = spawn_link(fun() ->
+ meck_mod:compile_and_load_forms(to_forms(Mod, NewExpects)),
+ gen_server:reply(From, ok) end),
+ Running = put(meck_reloading, Pid), %% XXX: don't do this.
+ Running =:= undefined orelse erlang:error(concurrent_reload),
NewExpects.
e_store(Expects, Func, Expect) ->
View
18 test/meck_tests.erl
@@ -490,6 +490,24 @@ passthrough_bif_test() ->
?assertEqual(ok, meck:new(file, [unstick, passthrough])),
?assertEqual(ok, meck:unload(file)).
+passthrough_file_bif_test_() ->
+ NeverExists = "/proc/0", %% 0 is invalid
+ AlwaysExists = "/proc/1", %% 1 is init
+ ?assertEqual({error, enoent}, file:read_file_info(NeverExists)),
+ ?assertMatch({ok, _}, file:read_file_info(AlwaysExists)),
+ {ok, ExistsInfo} = file:read_file_info(AlwaysExists),
+ {setup,local,
+ fun() -> ok = meck:new(file, [unstick, passthrough]) end,
+ fun(_) -> ok = meck:unload(file) end,
+ ?_test(begin
+ ?assertEqual(ok, meck:expect(file, read_file_info, fun
+ (Path) when Path =:= NeverExists -> {ok, no_info};
+ (Path) when Path =:= AlwaysExists -> meck:passthrough([Path]) end)),
+ ?assertEqual([], meck:history(file)),
+ ?assertEqual({ok, no_info}, file:read_file_info(NeverExists)),
+ ?assertEqual({ok, ExistsInfo}, file:read_file_info(AlwaysExists))
+ end)}.
+
cover_test() ->
{ok, _} = cover:compile("../test/meck_test_module.erl"),
a = meck_test_module:a(),
Something went wrong with that request. Please try again.