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
josevalim committed Dec 13, 2023
1 parent 27de623 commit 5d4fde6
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 58 deletions.
18 changes: 12 additions & 6 deletions lib/kernel/src/code.erl
View file
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
93 changes: 44 additions & 49 deletions lib/kernel/src/code_server.erl
View file
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,17 @@ 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 ->
case wait_loading(St0, Mod, From) of
{true, St1} -> {noreply, St1};
false -> get_object_code_for_loading(St0, Mod, From)
end
false when St0#state.mode =:= interactive ->
%% Handles pending on_load events first. If the code is being
%% loaded, finish before adding more entries to the queue.
Action = fun(_, St1) ->
case erlang:module_loaded(Mod) of
true -> {reply, {module, Mod}, St1};
false -> get_object_code_for_loading(St1, Mod, From)
end
end,
handle_pending_on_load(Action, Mod, From, St0);
false -> {reply, {error,embedded}, St0}
end;

handle_call(stop,_From, S) ->
Expand Down Expand Up @@ -1122,45 +1131,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 @@ -1191,22 +1189,16 @@ get_object_code(#state{path=Path} = St, Mod) when is_atom(Mod) ->
end.

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};
{error, St1} ->
%% Handles pending on_load events when we cannot find any
%% object code. It's possible that the user has loaded
%% a binary that has an on_load function, and in that case
%% we need to suspend them until the on_load function finishes.
handle_pending_on_load(
fun(_, St) ->
case erlang:module_loaded(Mod) of
true -> {reply, {module, Mod}, St};
false -> {reply, {error, nofile}, St}
end
end, Mod, From, St1)
case wait_loading(St0, Mod, From) of
{true, St1} -> {noreply, St1};
false ->
case get_object_code(St0, Mod) of
{Bin, FName, St1} ->
{Ref, St2} = monitor_loader(St1, Mod, From, Bin, FName),
{reply, {Bin, FName, Ref}, St2};
{error, St1} ->
{reply, {error, nofile}, St1}
end
end.

monitor_loader(#state{loading = Loading0} = St, Mod, Pid, Bin, FName) ->
Expand All @@ -1224,19 +1216,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 +1404,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
40 changes: 37 additions & 3 deletions lib/kernel/test/code_SUITE.erl
View file
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 All @@ -38,7 +39,7 @@
on_load_embedded/1, on_load_errors/1, on_load_update/1,
on_load_trace_on_load/1,
on_load_purge/1, on_load_self_call/1, on_load_pending/1,
on_load_deleted/1,
on_load_deleted/1, on_load_deadlock/1,
big_boot_embedded/1,
module_status/1,
get_mode/1, code_path_cache/1,
Expand All @@ -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 All @@ -72,7 +74,7 @@ all() ->
on_load_binary, on_load_embedded, on_load_errors,
{group, sequence},
on_load_purge, on_load_self_call, on_load_pending,
on_load_deleted,
on_load_deleted, on_load_deadlock,
module_status,
big_boot_embedded, get_mode, normalized_paths,
mult_embedded_flags].
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 Expand Up @@ -1901,6 +1910,31 @@ on_load_deleted(_Config) ->

ok.

on_load_deadlock(Config) ->
Mod = ?FUNCTION_NAME,
register(Mod, self()),
Tree = ?Q(["-module('@Mod@').\n",
"-export([ext/0]).\n",
"-on_load(f/0).\n",
"f() ->\n",
" timer:sleep(1000),\n",
" '@Mod@':ext().\n",
"ext() -> ok.\n"]),
merl:print(Tree),
{ok,Mod,Code} = merl:compile(Tree),

Dir = proplists:get_value(priv_dir, Config),
File = filename:join(Dir, atom_to_list(Mod) ++ ".beam"),
ok = file:write_file(File, Code),
code:add_path(Dir),

spawn(fun() -> code:ensure_loaded(Mod) end),
{error,Reason} = code:ensure_loaded(Mod),
true = (Reason =:= deadlock) or (Reason =:= on_load_failure),

code:del_path(Dir),
ok.

delete_before_reload(Mod, Reload) ->
false = check_old_code(Mod),

Expand Down

0 comments on commit 5d4fde6

Please sign in to comment.