From 33e73d357ae701d27c978dc53ef8eb0eb4864284 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Fri, 6 Oct 2023 18:55:39 +0200 Subject: [PATCH] Fix regressions in ensure_loaded 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 #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 #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. --- lib/kernel/src/code.erl | 18 +++++++---- lib/kernel/src/code_server.erl | 58 ++++++++++++++++------------------ lib/kernel/test/code_SUITE.erl | 11 ++++++- 3 files changed, 49 insertions(+), 38 deletions(-) diff --git a/lib/kernel/src/code.erl b/lib/kernel/src/code.erl index 86db90ed3a07..b2143bbfcb85 100644 --- a/lib/kernel/src/code.erl +++ b/lib/kernel/src/code.erl @@ -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 @@ -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. @@ -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; @@ -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; diff --git a/lib/kernel/src/code_server.erl b/lib/kernel/src/code_server.erl index c5ba677c6e85..6dfc95f57f4f 100644 --- a/lib/kernel/src/code_server.erl +++ b/lib/kernel/src/code_server.erl @@ -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 -> @@ -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) -> @@ -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 -> @@ -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 @@ -1224,11 +1217,14 @@ 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 @@ -1236,7 +1232,7 @@ loader_down(#state{loading = Loading0} = St, {Mod, Bin, FName}) -> 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), @@ -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. diff --git a/lib/kernel/test/code_SUITE.erl b/lib/kernel/test/code_SUITE.erl index 82ddfbf97905..cc9da8f35e29 100644 --- a/lib/kernel/test/code_SUITE.erl +++ b/lib/kernel/test/code_SUITE.erl @@ -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, @@ -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, @@ -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