From 957b6b1f6106371207525d83578539ba95e64a45 Mon Sep 17 00:00:00 2001 From: Mitchell Hanberg Date: Sat, 12 Aug 2023 19:48:03 -0400 Subject: [PATCH] Revert "fix: shutdown runtimes explicitly when LSP shutsdown" This reverts commit ca043803178eb0c477a2ddee796a9d9dfd522cdc. --- .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, 65 insertions(+), 138 deletions(-) diff --git a/.formatter.exs b/.formatter.exs index ada3d7cd..6ede60f6 100644 --- a/.formatter.exs +++ b/.formatter.exs @@ -12,7 +12,10 @@ plugins: [Styler], inputs: [ ".formatter.exs", - "{config,lib,test}/**/*.{ex,exs}", + "{config,lib,}/**/*.{ex,exs}", + "test/next_ls_test.exs", + "test/test_helper.exs", + "test/next_ls/**/*.{ex,exs}", "priv/**/*.ex" ] ] diff --git a/lib/next_ls.ex b/lib/next_ls.ex index 1d21e683..0b95e77d 100644 --- a/lib/next_ls.ex +++ b/lib/next_ls.ex @@ -317,10 +317,6 @@ 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 de934d0b..c71065f5 100644 --- a/lib/next_ls/runtime.ex +++ b/lib/next_ls/runtime.ex @@ -11,12 +11,27 @@ 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}, :infinity) + def call(server, mfa), do: GenServer.call(server, {:call, mfa}) - @spec shutdown(pid()) :: :ok - def shutdown(server), do: GenServer.call(server, :shutdown) + @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 compile(pid()) :: list() def compile(server) do GenServer.call(server, :compile, :infinity) end @@ -149,7 +164,6 @@ defmodule NextLS.Runtime do {:ok, %{ name: name, - node: nil, compiler_ref: nil, port: port, task_supervisor: task_supervisor, @@ -166,7 +180,7 @@ defmodule NextLS.Runtime do end @impl GenServer - def handle_call(:ready?, _from, %{node: nil} = state) do + def handle_call(:ready?, _from, %{node: _node} = state) do {:reply, true, state} end @@ -174,7 +188,7 @@ defmodule NextLS.Runtime do {:reply, false, state} end - def handle_call({:call, {m, f, a}}, _from, %{node: node} = state) when not is_nil(node) do + def handle_call({:call, {m, f, a}}, _from, %{node: node} = state) do reply = :rpc.call(node, m, f, a) {:reply, {:ok, reply}, state} end @@ -183,36 +197,18 @@ 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 -> - 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)}") + {_, errors} = :rpc.call(node, :_next_ls_private_compiler, :compile, []) - [] + Registry.dispatch(state.registry, :extensions, fn entries -> + for {pid, _} <- entries do + send(pid, {:compiler, errors}) + end + end) - {_, errors} -> - Registry.dispatch(state.registry, :extensions, fn entries -> - for {pid, _} <- entries do - send(pid, {:compiler, errors}) - end - end) - - errors - end + errors end) {:noreply, %{state | compiler_ref: %{task.ref => from}}} @@ -248,23 +244,6 @@ 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 234051c6..f78f3584 100644 --- a/test/next_ls/definition_test.exs +++ b/test/next_ls/definition_test.exs @@ -89,8 +89,6 @@ defmodule NextLS.DefinitionTest do }, "uri" => ^uri } - - shutdown(client) end test "go to imported function definition", %{client: client, bar: bar, imported: imported} = context do @@ -127,8 +125,6 @@ defmodule NextLS.DefinitionTest do }, "uri" => ^uri } - - shutdown(client) end test "go to remote function definition", %{client: client, bar: bar, remote: remote} = context do @@ -165,8 +161,6 @@ defmodule NextLS.DefinitionTest do }, "uri" => ^uri } - - shutdown(client) end end @@ -261,8 +255,6 @@ defmodule NextLS.DefinitionTest do }, "uri" => ^uri } - - shutdown(client) end test "go to imported macro definition", %{client: client, bar: bar, imported: imported} = context do @@ -299,8 +291,6 @@ defmodule NextLS.DefinitionTest do }, "uri" => ^uri } - - shutdown(client) end test "go to remote macro definition", %{client: client, bar: bar, remote: remote} = context do @@ -337,8 +327,6 @@ defmodule NextLS.DefinitionTest do }, "uri" => ^uri } - - shutdown(client) end end @@ -410,9 +398,8 @@ defmodule NextLS.DefinitionTest do } }, "uri" => ^uri - } - - shutdown(client) + }, + 500 end end end diff --git a/test/next_ls/diagnostics_test.exs b/test/next_ls/diagnostics_test.exs index 0fe18237..271c5903 100644 --- a/test/next_ls/diagnostics_test.exs +++ b/test/next_ls/diagnostics_test.exs @@ -112,8 +112,6 @@ 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 33e3ab82..e8e3e0bc 100644 --- a/test/next_ls/references_test.exs +++ b/test/next_ls/references_test.exs @@ -102,7 +102,5 @@ 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 590df7ce..178532c4 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_link_supervised!( + start_supervised!( {Runtime, name: "my_proj", on_initialized: on_init, @@ -61,6 +61,8 @@ defmodule NextLs.RuntimeTest do registry: RuntimeTest.Registry} ) + Process.link(pid) + assert wait_for_ready() assert {:ok, "\"hi\""} = Runtime.call(pid, {Kernel, :inspect, ["hi"]}) @@ -72,7 +74,7 @@ defmodule NextLs.RuntimeTest do tvisor = start_supervised!(Task.Supervisor) pid = - start_link_supervised!( + start_supervised!( {Runtime, task_supervisor: tvisor, name: "my_proj", @@ -85,6 +87,8 @@ defmodule NextLs.RuntimeTest do registry: RuntimeTest.Registry} ) + Process.link(pid) + assert {:error, :not_ready} = Runtime.call(pid, {IO, :puts, ["hi"]}) end @@ -142,38 +146,6 @@ 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 fb499cc7..af5189db 100644 --- a/test/next_ls/workspaces_test.exs +++ b/test/next_ls/workspaces_test.exs @@ -65,7 +65,6 @@ 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"] @@ -95,8 +94,6 @@ defmodule NextLS.WorkspacesTest do assert_notification "window/logMessage", %{ "message" => ^message } - - shutdown(client) end @tag root_paths: ["proj_one"] @@ -128,7 +125,6 @@ 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"] @@ -145,7 +141,5 @@ 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 48ed9e81..3a6e2674 100644 --- a/test/next_ls_test.exs +++ b/test/next_ls_test.exs @@ -56,21 +56,24 @@ 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") - shutdown(client) + assert :ok == + request(client, %{ + method: "shutdown", + id: 2, + jsonrpc: "2.0" + }) - Registry.dispatch(context.module, :runtimes, fn entries -> - for {pid, _} <- entries do - state = :sys.get_state(pid) - refute state.node - refute state.port - end - end) + assert_result 2, nil end test "returns method not found for unimplemented requests", %{client: client} do @@ -96,8 +99,21 @@ defmodule NextLSTest do "code" => -32_601, "message" => "Method Not Found: textDocument/signatureHelp" } + end - shutdown(client) + test "can initialize the server" do + assert_result 1, %{ + "capabilities" => %{ + "textDocumentSync" => %{ + "openClose" => true, + "save" => %{ + "includeText" => true + }, + "change" => 1 + } + }, + "serverInfo" => %{"name" => "Next LS"} + } end test "formats", %{client: client, cwd: cwd} = context do @@ -173,8 +189,6 @@ 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 @@ -219,7 +233,6 @@ defmodule NextLSTest do } assert_result 2, nil - shutdown(client) end test "workspace symbols", %{client: client, cwd: cwd} = context do @@ -311,8 +324,6 @@ defmodule NextLSTest do }, "name" => "defmodule Foo.CodeAction.NestedMod" } in symbols - - shutdown(client) end test "workspace symbols with query", %{client: client, cwd: cwd} = context do @@ -369,8 +380,6 @@ defmodule NextLSTest do "name" => "def foo" } ] == symbols - - shutdown(client) end test "deletes symbols when a file is deleted", %{client: client, cwd: cwd} = context do @@ -422,7 +431,6 @@ 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 fa15d13a..e77285a4 100644 --- a/test/support/utils.ex +++ b/test/support/utils.ex @@ -34,14 +34,6 @@ 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