From ca043803178eb0c477a2ddee796a9d9dfd522cdc Mon Sep 17 00:00:00 2001 From: Mitchell Hanberg Date: Sat, 12 Aug 2023 13:18:11 -0400 Subject: [PATCH] fix: shutdown runtimes explicitly when LSP shutsdown This should hopefully help with the tests, as they often spit out "blah blah overlapping partitions blah blah" with regard to the clustered nodes that are started. In test, we start many many nodes all connected to the main node, which is the test application. Copied some code from @zachallaun from #165 Co-authored-by: Zach Allaun --- .formatter.exs | 5 +- lib/next_ls.ex | 4 ++ lib/next_ls/runtime.ex | 77 ++++++++++++++++++++----------- test/next_ls/definition_test.exs | 17 ++++++- test/next_ls/diagnostics_test.exs | 2 + test/next_ls/references_test.exs | 2 + test/next_ls/runtime_test.exs | 40 +++++++++++++--- test/next_ls/workspaces_test.exs | 6 +++ test/next_ls_test.exs | 42 +++++++---------- test/support/utils.ex | 8 ++++ 10 files changed, 138 insertions(+), 65 deletions(-) diff --git a/.formatter.exs b/.formatter.exs index 6ede60f6..ada3d7cd 100644 --- a/.formatter.exs +++ b/.formatter.exs @@ -12,10 +12,7 @@ plugins: [Styler], inputs: [ ".formatter.exs", - "{config,lib,}/**/*.{ex,exs}", - "test/next_ls_test.exs", - "test/test_helper.exs", - "test/next_ls/**/*.{ex,exs}", + "{config,lib,test}/**/*.{ex,exs}", "priv/**/*.ex" ] ] diff --git a/lib/next_ls.ex b/lib/next_ls.ex index 0b95e77d..1d21e683 100644 --- a/lib/next_ls.ex +++ b/lib/next_ls.ex @@ -317,6 +317,10 @@ defmodule NextLS do end def handle_request(%Shutdown{}, lsp) do + dispatch(lsp.assigns.registry, :runtimes, fn entries -> + for {pid, _} <- entries, do: :ok = Runtime.shutdown(pid) + end) + {:reply, nil, assign(lsp, exit_code: 0)} end diff --git a/lib/next_ls/runtime.ex b/lib/next_ls/runtime.ex index d38cdc33..06839b7c 100644 --- a/lib/next_ls/runtime.ex +++ b/lib/next_ls/runtime.ex @@ -9,27 +9,12 @@ defmodule NextLS.Runtime do @type mod_fun_arg :: {atom(), atom(), list()} @spec call(pid(), mod_fun_arg()) :: any() - def call(server, mfa), do: GenServer.call(server, {:call, mfa}) + def call(server, mfa), do: GenServer.call(server, {:call, mfa}, :infinity) - @spec ready?(pid()) :: boolean() - def ready?(server), do: GenServer.call(server, :ready?) - - @spec await(pid(), non_neg_integer()) :: :ok | :timeout - def await(server, count \\ 50) - - def await(_server, 0) do - :timeout - end - - def await(server, count) do - if ready?(server) do - :ok - else - Process.sleep(500) - await(server, count - 1) - end - end + @spec shutdown(pid()) :: :ok + def shutdown(server), do: GenServer.call(server, :shutdown) + @spec compile(pid()) :: list() def compile(server) do GenServer.call(server, :compile, :infinity) end @@ -156,6 +141,7 @@ defmodule NextLS.Runtime do {:ok, %{ name: name, + node: nil, compiler_ref: nil, port: port, task_supervisor: task_supervisor, @@ -172,7 +158,7 @@ defmodule NextLS.Runtime do end @impl GenServer - def handle_call(:ready?, _from, %{node: _node} = state) do + def handle_call(:ready?, _from, %{node: nil} = state) do {:reply, true, state} end @@ -180,7 +166,7 @@ defmodule NextLS.Runtime do {:reply, false, state} end - def handle_call({:call, {m, f, a}}, _from, %{node: node} = state) do + def handle_call({:call, {m, f, a}}, _from, %{node: node} = state) when not is_nil(node) do reply = :rpc.call(node, m, f, a) {:reply, {:ok, reply}, state} end @@ -189,18 +175,36 @@ defmodule NextLS.Runtime do {:reply, {:error, :not_ready}, state} end + def handle_call(:shutdown, _from, state) do + with {:ok, node} <- Map.fetch(state, :node) do + Node.disconnect(node) + end + + with {:ok, port} <- Map.fetch(state, :port) do + Port.close(port) + end + + {:reply, :ok, %{state | node: nil, port: nil}} + end + def handle_call(:compile, from, %{node: node} = state) do task = Task.Supervisor.async_nolink(state.task_supervisor, fn -> - {_, errors} = :rpc.call(node, :_next_ls_private_compiler, :compile, []) + case :rpc.call(node, :_next_ls_private_compiler, :compile, []) do + {:badrpc, reason} -> + NextLS.Logger.warning(state.logger, "Bad RPC call to node #{node}: #{inspect(reason)}") - Registry.dispatch(state.registry, :extensions, fn entries -> - for {pid, _} <- entries do - send(pid, {:compiler, errors}) - end - end) + [] - errors + {_, errors} -> + Registry.dispatch(state.registry, :extensions, fn entries -> + for {pid, _} <- entries do + send(pid, {:compiler, errors}) + end + end) + + errors + end end) {:noreply, %{state | compiler_ref: %{task.ref => from}}} @@ -236,6 +240,23 @@ defmodule NextLS.Runtime do {:noreply, state} end + def handle_info({port, other}, %{port: port} = state) do + NextLS.Logger.log(state.logger, other) + {:noreply, state} + end + + def handle_info(msg, state) do + NextLS.Logger.log(state.logger, """ + Runtime process for #{state.name} received message: + + #{inspect(msg)}. + + You can safely ignore this log message. + """) + + {:noreply, state} + end + defp connect(_node, _port, 0) do false end diff --git a/test/next_ls/definition_test.exs b/test/next_ls/definition_test.exs index f78f3584..234051c6 100644 --- a/test/next_ls/definition_test.exs +++ b/test/next_ls/definition_test.exs @@ -89,6 +89,8 @@ defmodule NextLS.DefinitionTest do }, "uri" => ^uri } + + shutdown(client) end test "go to imported function definition", %{client: client, bar: bar, imported: imported} = context do @@ -125,6 +127,8 @@ defmodule NextLS.DefinitionTest do }, "uri" => ^uri } + + shutdown(client) end test "go to remote function definition", %{client: client, bar: bar, remote: remote} = context do @@ -161,6 +165,8 @@ defmodule NextLS.DefinitionTest do }, "uri" => ^uri } + + shutdown(client) end end @@ -255,6 +261,8 @@ defmodule NextLS.DefinitionTest do }, "uri" => ^uri } + + shutdown(client) end test "go to imported macro definition", %{client: client, bar: bar, imported: imported} = context do @@ -291,6 +299,8 @@ defmodule NextLS.DefinitionTest do }, "uri" => ^uri } + + shutdown(client) end test "go to remote macro definition", %{client: client, bar: bar, remote: remote} = context do @@ -327,6 +337,8 @@ defmodule NextLS.DefinitionTest do }, "uri" => ^uri } + + shutdown(client) end end @@ -398,8 +410,9 @@ defmodule NextLS.DefinitionTest do } }, "uri" => ^uri - }, - 500 + } + + shutdown(client) end end end diff --git a/test/next_ls/diagnostics_test.exs b/test/next_ls/diagnostics_test.exs index 271c5903..0fe18237 100644 --- a/test/next_ls/diagnostics_test.exs +++ b/test/next_ls/diagnostics_test.exs @@ -112,6 +112,8 @@ defmodule NextLS.DiagnosticsTest do } ] } + + shutdown(client) end end end diff --git a/test/next_ls/references_test.exs b/test/next_ls/references_test.exs index e8e3e0bc..33e3ab82 100644 --- a/test/next_ls/references_test.exs +++ b/test/next_ls/references_test.exs @@ -102,5 +102,7 @@ defmodule NextLS.ReferencesTest do } } ] + + shutdown(client) end end diff --git a/test/next_ls/runtime_test.exs b/test/next_ls/runtime_test.exs index 178532c4..590df7ce 100644 --- a/test/next_ls/runtime_test.exs +++ b/test/next_ls/runtime_test.exs @@ -48,7 +48,7 @@ defmodule NextLs.RuntimeTest do tvisor = start_supervised!(Task.Supervisor) pid = - start_supervised!( + start_link_supervised!( {Runtime, name: "my_proj", on_initialized: on_init, @@ -61,8 +61,6 @@ defmodule NextLs.RuntimeTest do registry: RuntimeTest.Registry} ) - Process.link(pid) - assert wait_for_ready() assert {:ok, "\"hi\""} = Runtime.call(pid, {Kernel, :inspect, ["hi"]}) @@ -74,7 +72,7 @@ defmodule NextLs.RuntimeTest do tvisor = start_supervised!(Task.Supervisor) pid = - start_supervised!( + start_link_supervised!( {Runtime, task_supervisor: tvisor, name: "my_proj", @@ -87,8 +85,6 @@ defmodule NextLs.RuntimeTest do registry: RuntimeTest.Registry} ) - Process.link(pid) - assert {:error, :not_ready} = Runtime.call(pid, {IO, :puts, ["hi"]}) end @@ -146,6 +142,38 @@ defmodule NextLs.RuntimeTest do end) =~ "Connected to node" end + test "shuts down the node and port", %{logger: logger, cwd: cwd, on_init: on_init} do + start_supervised!({Registry, keys: :duplicate, name: RuntimeTest.Registry}) + + tvisor = start_supervised!(Task.Supervisor) + + pid = + start_link_supervised!( + {Runtime, + name: "my_proj", + on_initialized: on_init, + task_supervisor: tvisor, + working_dir: cwd, + uri: "file://#{cwd}", + parent: self(), + logger: logger, + db: :some_db, + registry: RuntimeTest.Registry} + ) + + assert wait_for_ready() + + state = :sys.get_state(pid) + + assert state.node in Node.list() + assert Port.info(state.port) + + assert :ok == Runtime.shutdown(pid) + + assert state.node not in Node.list() + refute Port.info(state.port) + end + defp wait_for_ready do receive do :ready -> true diff --git a/test/next_ls/workspaces_test.exs b/test/next_ls/workspaces_test.exs index af5189db..fb499cc7 100644 --- a/test/next_ls/workspaces_test.exs +++ b/test/next_ls/workspaces_test.exs @@ -65,6 +65,7 @@ defmodule NextLS.WorkspacesTest do assert_is_ready(context, "proj_two") assert_notification "window/logMessage", %{"message" => "[NextLS] Compiled!"} + shutdown(client) end @tag root_paths: ["proj_one", "proj_two"] @@ -94,6 +95,8 @@ defmodule NextLS.WorkspacesTest do assert_notification "window/logMessage", %{ "message" => ^message } + + shutdown(client) end @tag root_paths: ["proj_one"] @@ -125,6 +128,7 @@ defmodule NextLS.WorkspacesTest do assert_is_ready(context, "proj_one") assert_notification "window/logMessage", %{"message" => "[NextLS] Compiled!"} + shutdown(client) end @tag root_paths: ["proj_one"] @@ -141,5 +145,7 @@ defmodule NextLS.WorkspacesTest do jsonrpc: "2.0", params: %{changes: [%{type: 3, uri: "file://#{Path.join(cwd, "proj_one/lib/peace.ex")}"}]} }) + + shutdown(client) end end diff --git a/test/next_ls_test.exs b/test/next_ls_test.exs index 3a6e2674..48ed9e81 100644 --- a/test/next_ls_test.exs +++ b/test/next_ls_test.exs @@ -56,24 +56,21 @@ defmodule NextLSTest do setup :with_lsp - test "can start the LSP server", %{server: server} do - assert alive?(server) - end - test "responds correctly to a shutdown request", %{client: client} = context do assert :ok == notify(client, %{method: "initialized", jsonrpc: "2.0", params: %{}}) assert_request(client, "client/registerCapability", fn _params -> nil end) assert_is_ready(context, "my_proj") - assert :ok == - request(client, %{ - method: "shutdown", - id: 2, - jsonrpc: "2.0" - }) + shutdown(client) - assert_result 2, nil + Registry.dispatch(context.module, :runtimes, fn entries -> + for {pid, _} <- entries do + state = :sys.get_state(pid) + refute state.node + refute state.port + end + end) end test "returns method not found for unimplemented requests", %{client: client} do @@ -99,21 +96,8 @@ defmodule NextLSTest do "code" => -32_601, "message" => "Method Not Found: textDocument/signatureHelp" } - end - test "can initialize the server" do - assert_result 1, %{ - "capabilities" => %{ - "textDocumentSync" => %{ - "openClose" => true, - "save" => %{ - "includeText" => true - }, - "change" => 1 - } - }, - "serverInfo" => %{"name" => "Next LS"} - } + shutdown(client) end test "formats", %{client: client, cwd: cwd} = context do @@ -189,6 +173,8 @@ defmodule NextLSTest do "range" => %{"start" => %{"character" => 0, "line" => 0}, "end" => %{"character" => 0, "line" => 8}} } ] + + shutdown(client) end test "formatting gracefully handles files with syntax errors", %{client: client, cwd: cwd} = context do @@ -233,6 +219,7 @@ defmodule NextLSTest do } assert_result 2, nil + shutdown(client) end test "workspace symbols", %{client: client, cwd: cwd} = context do @@ -324,6 +311,8 @@ defmodule NextLSTest do }, "name" => "defmodule Foo.CodeAction.NestedMod" } in symbols + + shutdown(client) end test "workspace symbols with query", %{client: client, cwd: cwd} = context do @@ -380,6 +369,8 @@ defmodule NextLSTest do "name" => "def foo" } ] == symbols + + shutdown(client) end test "deletes symbols when a file is deleted", %{client: client, cwd: cwd} = context do @@ -431,6 +422,7 @@ defmodule NextLSTest do assert_result 3, symbols assert symbol not in symbols + shutdown(client) end end end diff --git a/test/support/utils.ex b/test/support/utils.ex index e77285a4..fa15d13a 100644 --- a/test/support/utils.ex +++ b/test/support/utils.ex @@ -34,6 +34,14 @@ defmodule NextLS.Support.Utils do """ end + defmacro shutdown(client) do + quote do + assert :ok == request(unquote(client), %{method: "shutdown", id: 999, jsonrpc: "2.0"}) + + assert_result 999, nil + end + end + def with_lsp(%{tmp_dir: tmp_dir} = context) do root_paths = for path <- context[:root_paths] || [""] do