Do not crash debugger on on supported command (#294)

* do not crash on unsupported command in debugger

* remove not needed handle_info

the process is not trapping exits

* add a few matches on return values

* do not default to nil process name

* no need to start int as it's started by the debugger app

* add comment

* remove assertions that can never be met since the changes in 25e21ff

* reenable debugger tests

* add test

Author: lukaszsamson

apps/elixir_ls_debugger/lib/debugger/output.ex

@@ -13,14 +13,18 @@ defmodule ElixirLS.Debugger.Output do
 
   ## Client API
 
-  def start(name \\ nil) do
+  def start(name \\ __MODULE__) do
     GenServer.start(__MODULE__, :ok, name: name)
   end
 
   def send_response(server \\ __MODULE__, request_packet, response_body) do
     GenServer.call(server, {:send_response, request_packet, response_body})
   end
 
+  def send_error_response(server \\ __MODULE__, request_packet, message, format, variables) do
+    GenServer.call(server, {:send_error_response, request_packet, message, format, variables})
+  end
+
   def send_event(server \\ __MODULE__, event, body) do
     GenServer.call(server, {:send_event, event, body})
   end
@@ -46,7 +50,21 @@ defmodule ElixirLS.Debugger.Output do
     {:reply, :ok, seq + 1}
   end
 
-  @impl GenServer
+  def handle_call({:send_error_response, request_packet, message, format, variables}, _from, seq) do
+    send(
+      error_response(
+        seq,
+        request_packet["seq"],
+        request_packet["command"],
+        message,
+        format,
+        variables
+      )
+    )
+
+    {:reply, :ok, seq + 1}
+  end
+
   def handle_call({:send_event, event, body}, _from, seq) do
     send(event(seq, event, body))
     {:reply, :ok, seq + 1}

apps/elixir_ls_debugger/lib/debugger/protocol.basic.ex

@@ -36,6 +36,26 @@ defmodule ElixirLS.Debugger.Protocol.Basic do
     end
   end
 
+  defmacro error_response(seq, request_seq, command, message, format, variables) do
+    quote do
+      %{
+        "type" => "response",
+        "command" => unquote(command),
+        "seq" => unquote(seq),
+        "request_seq" => unquote(request_seq),
+        "success" => false,
+        "message" => unquote(message),
+        "body" => %{
+          "error" => %{
+            "id" => unquote(seq),
+            "format" => unquote(format),
+            "variables" => unquote(variables)
+          }
+        }
+      }
+    end
+  end
+
   defmacro event(seq, event, body) do
     quote do
       %{

apps/elixir_ls_debugger/lib/debugger/server.ex

@@ -10,6 +10,10 @@ defmodule ElixirLS.Debugger.Server do
   increment it any time we assign an ID.
   """
 
+  defmodule ServerError do
+    defexception [:message, :format, :variables]
+  end
+
   alias ElixirLS.Debugger.{Output, Stacktrace, Protocol, Variables}
   use GenServer
   use Protocol
@@ -54,16 +58,21 @@ defmodule ElixirLS.Debugger.Server do
 
   @impl GenServer
   def init(opts) do
-    {:ok, _pid} = :int.start()
     state = if opts[:output], do: %__MODULE__{output: opts[:output]}, else: %__MODULE__{}
     {:ok, state}
   end
 
   @impl GenServer
   def handle_cast({:receive_packet, request(_, _) = packet}, state) do
-    {response_body, state} = handle_request(packet, state)
-    Output.send_response(packet, response_body)
-    {:noreply, state}
+    try do
+      {response_body, state} = handle_request(packet, state)
+      Output.send_response(packet, response_body)
+      {:noreply, state}
+    rescue
+      e in ServerError ->
+        Output.send_error_response(packet, e.message, e.format, e.variables)
+        {:noreply, state}
+    end
   end
 
   @impl GenServer
@@ -79,6 +88,7 @@ defmodule ElixirLS.Debugger.Server do
     {:noreply, state}
   end
 
+  # the `:DOWN` message is not delivered under normal conditions as the process calls `Process.sleep(:infinity)`
   @impl GenServer
   def handle_info({:DOWN, ref, :process, _pid, reason}, %{task_ref: ref} = state) do
     exit_code =
@@ -101,11 +111,6 @@ defmodule ElixirLS.Debugger.Server do
     {:noreply, %{state | task_ref: nil}}
   end
 
-  @impl GenServer
-  def handle_info({:EXIT, _, :normal}, state) do
-    {:noreply, state}
-  end
-
   # If we get the disconnect request from the client, we send :disconnect to the server so it will
   # die right after responding to the request
   @impl GenServer
@@ -293,7 +298,7 @@ defmodule ElixirLS.Debugger.Server do
     pid = state.threads[thread_id]
     state = remove_paused_process(state, pid)
 
-    :int.continue(pid)
+    :ok = :int.continue(pid)
     {%{"allThreadsContinued" => false}, state}
   end
 
@@ -321,6 +326,15 @@ defmodule ElixirLS.Debugger.Server do
     {%{}, state}
   end
 
+  defp handle_request(request(_, command), _state) when is_binary(command) do
+    raise ServerError,
+      message: "notSupported",
+      format: "Debugger request {command} is currently not supported",
+      variables: %{
+        "command" => command
+      }
+  end
+
   defp remove_paused_process(state, pid) do
     update_in(state.paused_processes, fn paused_processes ->
       Map.delete(paused_processes, pid)
@@ -530,7 +544,7 @@ defmodule ElixirLS.Debugger.Server do
     |> Enum.filter(&interpretable?(&1, exclude_modules))
     |> Enum.map(fn mod ->
       try do
-        :int.ni(mod)
+        {:module, _} = :int.ni(mod)
       catch
         _, _ ->
           IO.warn(
@@ -609,8 +623,8 @@ defmodule ElixirLS.Debugger.Server do
 
   defp save_and_reload(module, beam_bin) do
     :ok = File.write(Path.join(@temp_beam_dir, to_string(module) <> ".beam"), beam_bin)
-    :code.delete(module)
-    :int.ni(module)
+    true = :code.delete(module)
+    {:module, _} = :int.ni(module)
   end
 
   defp set_breakpoints(path, lines) do
@@ -640,7 +654,7 @@ defmodule ElixirLS.Debugger.Server do
   defp set_breakpoint(module, line) do
     case :int.ni(module) do
       {:module, _} ->
-        :int.break(module, line)
+        :ok = :int.break(module, line)
         {:ok, module, line}
 
       _ ->

apps/elixir_ls_debugger/test/debugger_test.exs

@@ -26,8 +26,6 @@ defmodule ElixirLS.Debugger.ServerTest do
     {:ok, %{server: server}}
   end
 
-  # Causes test suite not to finish
-  @tag :pending
   test "basic debugging", %{server: server} do
     in_fixture(__DIR__, "mix_project", fn ->
       Server.receive_packet(server, initialize_req(1, %{}))
@@ -128,13 +126,19 @@ defmodule ElixirLS.Debugger.ServerTest do
       Server.receive_packet(server, continue_req(10, thread_id))
       assert_receive response(_, 10, "continue", %{"allThreadsContinued" => false})
 
-      assert_receive(event(_, "exited", %{"exitCode" => 0}))
-      assert_receive(event(_, "terminated", %{"restart" => false}))
+      Server.receive_packet(server, request(11, "someRequest", %{"threadId" => 123}))
+
+      assert_receive error_response(
+                       _,
+                       11,
+                       "someRequest",
+                       "notSupported",
+                       "Debugger request {command} is currently not supported",
+                       %{"command" => "someRequest"}
+                     )
     end)
   end
 
-  # Causes test suite not to finish
-  @tag :pending
   test "sets breakpoints in erlang modules", %{server: server} do
     in_fixture(__DIR__, "mix_project", fn ->
       Server.receive_packet(server, initialize_req(1, %{}))