Skip to content

Commit

Permalink
Fix regressions in ensure_loaded
Browse files Browse the repository at this point in the history
The first regression is not attempt to
load code if mode is embedded.

Erlang/OTP 25 only attempted to perform
code loading if the mode was interactive:

https://github.com/erlang/otp/blob/maint-25/lib/kernel/src/code_server.erl#L301

This check was removed in erlang#6736 as part of
the decentralization. However, we received
reports of increased cpu/memory usage in
Erlang/OTP 26.1 in a code that was calling
code:ensure_loaded/1 on a hot path. The
underlying code was fixed but, given erlang#7503
added the server back into the equation for
ensure_loaded we can add the mode check
back to preserve Erlang/OTP 25 behaviour.

The second regression would cause the caller
process to deadlock if attempting to load a
file with invalid .beam more than once.
  • Loading branch information
josevalim committed Oct 15, 2023
1 parent 3a914d2 commit 33e73d3
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 38 deletions.
18 changes: 12 additions & 6 deletions lib/kernel/src/code.erl
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ objfile_extension() ->
load_file(Mod) when is_atom(Mod) ->
case get_object_code(Mod) of
error -> {error,nofile};
{Mod,Binary,File} -> load_module(Mod, File, Binary, false, false)
{Mod,Binary,File} -> load_module(Mod, File, Binary, false)
end.

-spec ensure_loaded(Module) -> {module, Module} | {error, What} when
Expand All @@ -190,7 +190,13 @@ ensure_loaded(Mod) when is_atom(Mod) ->
case call({get_object_code_for_loading, Mod}) of
{module, Mod} -> {module, Mod};
{error, What} -> {error, What};
{Mod,Binary,File,Ref} -> load_module(Mod, File, Binary, false, Ref)
{Binary,File,Ref} ->
case erlang:prepare_loading(Mod, Binary) of
{error,_}=Error ->
call({load_error, Ref, Mod, Error});
Prepared ->
call({load_module, Prepared, Mod, File, false, Ref})
end
end
end.

Expand All @@ -209,7 +215,7 @@ load_abs(File, M) when (is_list(File) orelse is_atom(File)), is_atom(M) ->
FileName = code_server:absname(FileName0),
case erl_prim_loader:get_file(FileName) of
{ok,Bin,_} ->
load_module(M, FileName, Bin, false, false);
load_module(M, FileName, Bin, false);
error ->
{error, nofile}
end;
Expand All @@ -227,16 +233,16 @@ load_abs(File, M) when (is_list(File) orelse is_atom(File)), is_atom(M) ->
load_binary(Mod, File, Bin)
when is_atom(Mod), (is_list(File) orelse is_atom(File)), is_binary(Bin) ->
case modp(File) of
true -> load_module(Mod, File, Bin, true, false);
true -> load_module(Mod, File, Bin, true);
false -> {error,badarg}
end.

load_module(Mod, File, Bin, Purge, EnsureLoaded) ->
load_module(Mod, File, Bin, Purge) ->
case erlang:prepare_loading(Mod, Bin) of
{error,_}=Error ->
Error;
Prepared ->
call({load_module, Prepared, Mod, File, Purge, EnsureLoaded})
call({load_module, Prepared, Mod, File, Purge, false})
end.

modp(Atom) when is_atom(Atom) -> true;
Expand Down
58 changes: 27 additions & 31 deletions lib/kernel/src/code_server.erl
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,9 @@ handle_call({load_module,PC,Mod,File,Purge,EnsureLoaded}, From, S)
end,
try_finish_module(File, Mod, PC, EnsureLoaded, From, S);

handle_call({load_error,Ref,Mod,Error}, _From, S) ->
reply_loading(Ref, Mod, Error, S);

handle_call({delete,Mod}, _From, St) when is_atom(Mod) ->
case catch erlang:delete_module(Mod) of
true ->
Expand Down Expand Up @@ -339,11 +342,12 @@ handle_call({get_object_code,Mod}, _From, St0) when is_atom(Mod) ->
handle_call({get_object_code_for_loading,Mod}, From, St0) when is_atom(Mod) ->
case erlang:module_loaded(Mod) of
true -> {reply, {module, Mod}, St0};
false ->
false when St0#state.mode =:= interactive ->
case wait_loading(St0, Mod, From) of
{true, St1} -> {noreply, St1};
false -> get_object_code_for_loading(St0, Mod, From)
end
end;
false -> {reply, {error,embedded}, St0}
end;

handle_call(stop,_From, S) ->
Expand Down Expand Up @@ -1122,45 +1126,34 @@ try_finish_module(File, Mod, PC, EnsureLoaded, From, St) ->
_ ->
fun(_, S0) ->
case erlang:module_loaded(Mod) of
true when is_reference(EnsureLoaded) ->
S = done_loading(St, Mod, EnsureLoaded, {module, Mod}),
{reply,{module,Mod},S};
true ->
{reply,{module,Mod},S0};
reply_loading(EnsureLoaded, Mod, {module, Mod}, St);
false when S0#state.mode =:= interactive ->
try_finish_module_1(File, Mod, PC, From, EnsureLoaded, S0);
false ->
{reply,{error,embedded},S0}
reply_loading(EnsureLoaded, Mod, {error, embedded}, St)
end
end
end,
handle_pending_on_load(Action, Mod, From, St).

try_finish_module_1(File, Mod, PC, From, EnsureRef, #state{moddb=Db}=St) ->
try_finish_module_1(File, Mod, PC, From, EnsureLoaded, #state{moddb=Db}=St) ->
case is_sticky(Mod, Db) of
true -> %% Sticky file reject the load
error_msg("Can't load module '~w' that resides in sticky dir\n",[Mod]),
{reply,{error,sticky_directory},St};
reply_loading(EnsureLoaded, Mod, {error,sticky_directory}, St);
false ->
try_finish_module_2(File, Mod, PC, From, EnsureRef, St)
try_finish_module_2(File, Mod, PC, From, EnsureLoaded, St)
end.

try_finish_module_2(File, Mod, PC, From, EnsureRef, St0) ->
Action = fun(Result, #state{moddb=Db}=S0) ->
S = case is_reference(EnsureRef) of
true -> done_loading(S0, Mod, EnsureRef, Result);
false -> S0
end,
try_finish_module_2(File, Mod, PC, From, EnsureLoaded, St0) ->
Action = fun(Result, #state{moddb=Db}=St1) ->
case Result of
{module, _} = Module ->
ets:insert(Db, {Mod, File}),
{reply, Module, S};
{error, on_load_failure} = Error ->
{reply, Error, S};
{error, What} = Error ->
error_msg("Loading of ~ts failed: ~p\n", [File, What]),
{reply, Error, S}
end
{module, _} -> ets:insert(Db, {Mod, File});
{error, on_load_failure} -> ok;
{error, What} -> error_msg("Loading of ~ts failed: ~p\n", [File, What])
end,
reply_loading(EnsureLoaded, Mod, Result, St1)
end,
Res = case erlang:finish_loading([PC]) of
ok ->
Expand Down Expand Up @@ -1194,7 +1187,7 @@ get_object_code_for_loading(St0, Mod, From) ->
case get_object_code(St0, Mod) of
{Bin, FName, St1} ->
{Ref, St2} = monitor_loader(St1, Mod, From, Bin, FName),
{reply, {Mod, Bin, FName, Ref}, St2};
{reply, {Bin, FName, Ref}, St2};
{error, St1} ->
%% Handles pending on_load events when we cannot find any
%% object code. It's possible that the user has loaded
Expand Down Expand Up @@ -1224,19 +1217,22 @@ wait_loading(#state{loading = Loading0} = St, Mod, Pid) ->
false
end.

done_loading(#state{loading = Loading0} = St, Mod, Ref, Res) ->
reply_loading(Ref, Mod, Reply, #state{loading = Loading0} = St)
when is_reference(Ref) ->
{Waiting, Loading} = maps:take(Mod, Loading0),
_ = [reply(Pid, Res) || Pid <- Waiting],
_ = [reply(Pid, Reply) || Pid <- Waiting],
erlang:demonitor(Ref, [flush]),
St#state{loading = Loading}.
{reply, Reply, St#state{loading = Loading}};
reply_loading(Ref, _Mod, Reply, St) when is_boolean(Ref) ->
{reply, Reply, St}.

loader_down(#state{loading = Loading0} = St, {Mod, Bin, FName}) ->
case Loading0 of
#{Mod := [First | Rest]} ->
Tag = {'LOADER_DOWN', {Mod, Bin, FName}},
Ref = erlang:monitor(process, First, [{tag, Tag}]),
Loading = Loading0#{Mod := Rest},
_ = reply(First, {Mod, Bin, FName, Ref}),
_ = reply(First, {Bin, FName, Ref}),
St#state{loading = Loading};
#{Mod := []} ->
Loading = maps:remove(Mod, Loading0),
Expand Down Expand Up @@ -1409,7 +1405,7 @@ handle_on_load(Res, Action, _, _, St) ->
handle_pending_on_load(Action, Mod, From, #state{on_load=OnLoad0}=St) ->
case lists:keyfind(Mod, 2, OnLoad0) of
false ->
Action(ok, St);
Action({module, Mod}, St);
{{From,_Ref},Mod,_Pids} ->
%% The on_load function tried to make an external
%% call to its own module. That would be a deadlock.
Expand Down
11 changes: 10 additions & 1 deletion lib/kernel/test/code_SUITE.erl
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@
-export([all/0, suite/0,groups/0,init_per_group/2,end_per_group/2]).
-export([set_path/1, get_path/1, add_path/1, add_paths/1, del_path/1,
replace_path/1, load_file/1, load_abs/1,
ensure_loaded/1, ensure_loaded_many/1, ensure_loaded_many_kill/1,
ensure_loaded/1, ensure_loaded_many/1, ensure_loaded_many_kill/1,
ensure_loaded_bad_case/1,
delete/1, purge/1, purge_many_exits/0, purge_many_exits/1,
soft_purge/1, is_loaded/1, all_loaded/1, all_available/1,
load_binary/1, dir_req/1, object_code/1, set_path_file/1,
Expand Down Expand Up @@ -62,6 +63,7 @@ all() ->
[set_path, get_path, add_path, add_paths, del_path,
replace_path, load_file, load_abs,
ensure_loaded, ensure_loaded_many, ensure_loaded_many_kill,
ensure_loaded_bad_case,
delete, purge, purge_many_exits, soft_purge, is_loaded, all_loaded,
all_available, load_binary, dir_req, object_code, set_path_file,
upgrade, code_path_cache,
Expand Down Expand Up @@ -416,6 +418,13 @@ load_abs(Config) when is_list(Config) ->
code:unstick_dir(TestDir),
ok.

ensure_loaded_bad_case(Config) when is_list(Config) ->
%% Make sure loading an invalid .beam file does
%% not hold a permanent lock on the server.
{error, _} = code:ensure_loaded('STRING'),
{error, _} = code:ensure_loaded('STRING'),
ok.

ensure_loaded(Config) when is_list(Config) ->
{module, lists} = code:ensure_loaded(lists),
case init:get_argument(mode) of
Expand Down

0 comments on commit 33e73d3

Please sign in to comment.