From 0481fb868a2cc6f3df4cfa9a33bd6899cc1d7144 Mon Sep 17 00:00:00 2001 From: Lukasz Samson Date: Tue, 11 Jul 2023 06:16:33 +0200 Subject: [PATCH] cache module_info of interpreted modules call to module_info may deadlock when stopped on a breakpoint Fixes https://github.com/elixir-lsp/elixir-ls/issues/940 --- .../lib/debugger/module_info_cache.ex | 30 +++++++ .../elixir_ls_debugger/lib/debugger/server.ex | 11 ++- .../lib/debugger/stacktrace.ex | 5 +- .../elixir_ls_debugger/test/debugger_test.exs | 90 ++++++++++++++++++- .../mix_project/lib/protocol_breakpoints.ex | 29 ++++++ 5 files changed, 159 insertions(+), 6 deletions(-) create mode 100644 apps/elixir_ls_debugger/lib/debugger/module_info_cache.ex diff --git a/apps/elixir_ls_debugger/lib/debugger/module_info_cache.ex b/apps/elixir_ls_debugger/lib/debugger/module_info_cache.ex new file mode 100644 index 000000000..48b21b89e --- /dev/null +++ b/apps/elixir_ls_debugger/lib/debugger/module_info_cache.ex @@ -0,0 +1,30 @@ +defmodule ElixirLS.Debugger.ModuleInfoCache do + @moduledoc """ + Caches module_info of interpreted modules. There are cases when module_info call + may deadlock (https://github.com/elixir-lsp/elixir-ls/issues/940) + """ + + use Agent + + def start_link(args) do + Agent.start_link(fn -> args end, name: __MODULE__) + end + + def get(module) do + Agent.get(__MODULE__, & &1[module]) + end + + def store(module) do + Agent.update(__MODULE__, fn map -> + if Map.has_key?(map, module) do + map + else + Map.put(map, module, module.module_info()) + end + end) + end + + def clear() do + Agent.update(__MODULE__, fn _map -> %{} end) + end +end diff --git a/apps/elixir_ls_debugger/lib/debugger/server.ex b/apps/elixir_ls_debugger/lib/debugger/server.ex index 347d2e5ac..9611952be 100644 --- a/apps/elixir_ls_debugger/lib/debugger/server.ex +++ b/apps/elixir_ls_debugger/lib/debugger/server.ex @@ -23,7 +23,8 @@ defmodule ElixirLS.Debugger.Server do Variables, Utils, BreakpointCondition, - Binding + Binding, + ModuleInfoCache } alias ElixirLS.Debugger.Stacktrace.Frame @@ -84,6 +85,7 @@ defmodule ElixirLS.Debugger.Server do @impl GenServer def init(opts) do BreakpointCondition.start_link([]) + ModuleInfoCache.start_link(%{}) state = if opts[:output], do: %__MODULE__{output: opts[:output]}, else: %__MODULE__{} {:ok, state} end @@ -335,6 +337,7 @@ defmodule ElixirLS.Debugger.Server do nil -> case :int.ni(m) do {:module, _} -> + ModuleInfoCache.store(m) breaks_before = :int.all_breaks(m) Output.debugger_console( @@ -1152,6 +1155,7 @@ defmodule ElixirLS.Debugger.Server do true = :code.delete(module) Output.debugger_console("Interpreting module #{inspect(module)}") {:module, _} = :int.ni(module) + ModuleInfoCache.store(module) end defp set_breakpoints(path, lines) do @@ -1164,7 +1168,8 @@ defmodule ElixirLS.Debugger.Server do |> Enum.map(&elem(&1, 0)) |> Enum.filter(fn module -> String.starts_with?(Atom.to_string(module), "Elixir.") end) |> Enum.group_by(fn module -> - Path.expand(to_string(module.module_info(:compile)[:source])) + module_info = ModuleInfoCache.get(module) || module.module_info() + Path.expand(to_string(module_info[:compile][:source])) end) loaded_modules_from_path = Map.get(loaded_elixir_modules, path, []) @@ -1199,6 +1204,7 @@ defmodule ElixirLS.Debugger.Server do Enum.reduce(modules, [], fn module, added -> case :int.ni(module) do {:module, _} -> + ModuleInfoCache.store(module) Output.debugger_console("Setting breakpoint in #{inspect(module)} #{path}:#{line}") :int.break(module, line) update_break_condition(module, line, condition, log_message, hit_count) @@ -1233,6 +1239,7 @@ defmodule ElixirLS.Debugger.Server do try do Output.debugger_console("Interpreting module #{inspect(mod)}") {:module, _} = :int.ni(mod) + ModuleInfoCache.store(mod) catch _, _ -> Output.debugger_important( diff --git a/apps/elixir_ls_debugger/lib/debugger/stacktrace.ex b/apps/elixir_ls_debugger/lib/debugger/stacktrace.ex index 549a5b3da..1886d1fc0 100644 --- a/apps/elixir_ls_debugger/lib/debugger/stacktrace.ex +++ b/apps/elixir_ls_debugger/lib/debugger/stacktrace.ex @@ -3,6 +3,7 @@ defmodule ElixirLS.Debugger.Stacktrace do Retrieves the stack trace for a process that's paused at a breakpoint """ alias ElixirLS.Debugger.Output + alias ElixirLS.Debugger.ModuleInfoCache defmodule Frame do defstruct [:level, :file, :module, :function, :args, :line, :bindings, :messages] @@ -86,8 +87,6 @@ defmodule ElixirLS.Debugger.Stacktrace do end defp get_file(module) do - if Code.ensure_loaded?(module) do - to_string(module.module_info[:compile][:source]) - end + Path.expand(to_string(ModuleInfoCache.get(module)[:compile][:source])) end end diff --git a/apps/elixir_ls_debugger/test/debugger_test.exs b/apps/elixir_ls_debugger/test/debugger_test.exs index 21c5f0f4e..41c040b5b 100644 --- a/apps/elixir_ls_debugger/test/debugger_test.exs +++ b/apps/elixir_ls_debugger/test/debugger_test.exs @@ -4,7 +4,7 @@ defmodule ElixirLS.Debugger.ServerTest do # between the debugger's tests and the fixture project's tests. Expect to see output printed # from both. - alias ElixirLS.Debugger.{Server, Protocol, BreakpointCondition} + alias ElixirLS.Debugger.{Server, Protocol, BreakpointCondition, ModuleInfoCache} use ElixirLS.Utils.MixTest.Case, async: false use Protocol @@ -22,6 +22,7 @@ defmodule ElixirLS.Debugger.ServerTest do :int.no_break() :int.clear() BreakpointCondition.clear() + ModuleInfoCache.clear() end) {:ok, %{server: server}} @@ -2105,6 +2106,93 @@ defmodule ElixirLS.Debugger.ServerTest do assert %{} == :sys.get_state(server).function_breakpoints end) end + + @tag :fixture + test "function breakpoint in derived protocol implementation", %{server: server} do + in_fixture(__DIR__, "mix_project", fn -> + Server.receive_packet( + server, + initialize_req(1, %{ + "supportsVariablePaging" => true, + "supportsVariableType" => true + }) + ) + + assert_receive( + response(_, 1, "initialize", %{"supportsConfigurationDoneRequest" => true}) + ) + + Server.receive_packet( + server, + launch_req(2, %{ + "request" => "launch", + "type" => "mix_task", + "task" => "run", + "taskArgs" => ["-e", "ProtocolBreakpoints.go2()"], + "projectDir" => File.cwd!() + }) + ) + + assert_receive(response(_, 2, "launch", %{}), 5000) + assert_receive(event(_, "initialized", %{})) + abs_path = Path.absname("lib/protocol_breakpoints.ex") + + Server.receive_packet( + server, + set_function_breakpoints_req(3, [%{"name" => "DerivedProto.MyStruct.go/1"}]) + ) + + assert_receive( + response(_, 3, "setFunctionBreakpoints", %{"breakpoints" => [%{"verified" => true}]}), + 3000 + ) + + assert DerivedProto.MyStruct in :int.interpreted() + + assert [ + {{DerivedProto.MyStruct, 33}, + [:active, :enable, :null, {ElixirLS.Debugger.BreakpointCondition, :check_0}]} + ] = :int.all_breaks(DerivedProto.MyStruct) + + assert %{{DerivedProto.MyStruct, :go, 1} => [33]} = + :sys.get_state(server).function_breakpoints + + Server.receive_packet(server, request(5, "configurationDone", %{})) + assert_receive(response(_, 5, "configurationDone", %{})) + + assert_receive event(_, "stopped", %{ + "allThreadsStopped" => false, + "reason" => "function breakpoint", + "threadId" => thread_id + }), + 5_000 + + Server.receive_packet(server, stacktrace_req(7, thread_id)) + + assert_receive response(_, 7, "stackTrace", %{ + "stackFrames" => [ + %{ + "column" => 0, + "id" => frame_id, + "line" => 33, + "name" => "DerivedProto.MyStruct.go/1", + "source" => %{"path" => ^abs_path} + } + ] + }) + when is_integer(frame_id) + + Server.receive_packet( + server, + set_function_breakpoints_req(9, []) + ) + + assert_receive(response(_, 9, "setFunctionBreakpoints", %{"breakpoints" => []})) + + assert [] = :int.all_breaks(DerivedProto.MyStruct) + assert %{} == :sys.get_state(server).function_breakpoints + end) + end end @tag :fixture diff --git a/apps/elixir_ls_debugger/test/fixtures/mix_project/lib/protocol_breakpoints.ex b/apps/elixir_ls_debugger/test/fixtures/mix_project/lib/protocol_breakpoints.ex index bd6cee19f..85a96bef0 100644 --- a/apps/elixir_ls_debugger/test/fixtures/mix_project/lib/protocol_breakpoints.ex +++ b/apps/elixir_ls_debugger/test/fixtures/mix_project/lib/protocol_breakpoints.ex @@ -8,9 +8,38 @@ defimpl Proto, for: [List, BitString] do end end +defprotocol DerivedProto do + def go(t) +end + +defimpl DerivedProto, for: Any do + defmacro __deriving__(module, _struct, _options) do + quote do + defimpl unquote(@protocol), for: unquote(module) do + def go(term) do + IO.puts("") + end + end + end + end + + def go(term) do + raise Protocol.UndefinedError, protocol: @protocol, value: term + end +end + +defmodule MyStruct do + @derive [{DerivedProto, []}] + defstruct [:a] +end + defmodule ProtocolBreakpoints do def go1() do Proto.go([]) Proto.go("") end + + def go2() do + DerivedProto.go(%MyStruct{}) + end end