Spec compliance improvements (#399)

* Improve spec compliance when starting and shutting down

add startup/shutdown tests

* normalize handling of $/ messages

* fix test crash

* handle protocol errors in packet stream

* increase spec compatibility in debugger

* server should not send requests until client notifies initialized

* Update apps/language_server/lib/language_server/server.ex

Co-authored-by: Jason Axelson <axelson@users.noreply.github.com>

Author: lukaszsamson

apps/elixir_ls_debugger/lib/debugger/server.ex

@@ -64,11 +64,33 @@ defmodule ElixirLS.Debugger.Server do
   end
 
   @impl GenServer
+  def handle_cast({:receive_packet, request(_, "disconnect") = packet}, state) do
+    Output.send_response(packet, %{})
+    {:noreply, state, {:continue, :disconnect}}
+  end
+
   def handle_cast({:receive_packet, request(_, _) = packet}, state) do
     try do
-      {response_body, state} = handle_request(packet, state)
-      Output.send_response(packet, response_body)
-      {:noreply, state}
+      if state.client_info == nil do
+        case packet do
+          request(_, "initialize") ->
+            {response_body, state} = handle_request(packet, state)
+            Output.send_response(packet, response_body)
+            {:noreply, state}
+
+          request(_, command) ->
+            raise ServerError,
+              message: "invalidRequest",
+              format: "Debugger request {command} was not expected",
+              variables: %{
+                "command" => command
+              }
+        end
+      else
+        {response_body, state} = handle_request(packet, state)
+        Output.send_response(packet, response_body)
+        {:noreply, state}
+      end
     rescue
       e in ServerError ->
         Output.send_error_response(packet, e.message, e.format, e.variables)
@@ -112,11 +134,16 @@ defmodule ElixirLS.Debugger.Server do
     {:noreply, %{state | task_ref: nil}}
   end
 
-  # If we get the disconnect request from the client, we send :disconnect to the server so it will
+  # If we get the disconnect request from the client, we continue with :disconnect so the server will
   # die right after responding to the request
   @impl GenServer
-  def handle_info(:disconnect, state) do
-    System.halt(0)
+  def handle_continue(:disconnect, state) do
+    unless Application.get_env(:elixir_ls_debugger, :test_mode) do
+      System.halt(0)
+    else
+      Process.exit(self(), {:exit_code, 0})
+    end
+
     {:noreply, state}
   end
 
@@ -129,10 +156,19 @@ defmodule ElixirLS.Debugger.Server do
 
   ## Helpers
 
-  defp handle_request(initialize_req(_, client_info), state) do
+  defp handle_request(initialize_req(_, client_info), %{client_info: nil} = state) do
     {capabilities(), %{state | client_info: client_info}}
   end
 
+  defp handle_request(initialize_req(_, _client_info), _state) do
+    raise ServerError,
+      message: "invalidRequest",
+      format: "Debugger request {command} was not expected",
+      variables: %{
+        "command" => "initialize"
+      }
+  end
+
   defp handle_request(launch_req(_, config), state) do
     {_, ref} = spawn_monitor(fn -> initialize(config) end)
 
@@ -293,11 +329,6 @@ defmodule ElixirLS.Debugger.Server do
     {%{"result" => inspect(result), "variablesReference" => 0}, state}
   end
 
-  defp handle_request(request(_, "disconnect"), state) do
-    send(self(), :disconnect)
-    {%{}, state}
-  end
-
   defp handle_request(continue_req(_, thread_id), state) do
     pid = state.threads[thread_id]
     state = remove_paused_process(state, pid)

apps/elixir_ls_debugger/test/debugger_test.exs

@@ -26,6 +26,69 @@ defmodule ElixirLS.Debugger.ServerTest do
     {:ok, %{server: server}}
   end
 
+  describe "initialize" do
+    test "succeedes", %{server: server} do
+      Server.receive_packet(server, initialize_req(1, %{"clientID" => "some_id"}))
+      assert_receive(response(_, 1, "initialize", %{"supportsConfigurationDoneRequest" => true}))
+      assert :sys.get_state(server).client_info == %{"clientID" => "some_id"}
+    end
+
+    test "fails when already initialized", %{server: server} do
+      Server.receive_packet(server, initialize_req(1, %{"clientID" => "some_id"}))
+      assert_receive(response(_, 1, "initialize", %{"supportsConfigurationDoneRequest" => true}))
+      Server.receive_packet(server, initialize_req(2, %{"clientID" => "some_id"}))
+
+      assert_receive(
+        error_response(
+          _,
+          2,
+          "initialize",
+          "invalidRequest",
+          "Debugger request {command} was not expected",
+          %{"command" => "initialize"}
+        )
+      )
+    end
+
+    test "rejects requests when not initialized", %{server: server} do
+      Server.receive_packet(
+        server,
+        set_breakpoints_req(1, %{"path" => "lib/mix_project.ex"}, [%{"line" => 3}])
+      )
+
+      assert_receive(
+        error_response(
+          _,
+          1,
+          "setBreakpoints",
+          "invalidRequest",
+          "Debugger request {command} was not expected",
+          %{"command" => "setBreakpoints"}
+        )
+      )
+    end
+  end
+
+  describe "disconnect" do
+    test "succeedes when not initialized", %{server: server} do
+      Process.flag(:trap_exit, true)
+      Server.receive_packet(server, request(1, "disconnect"))
+      assert_receive(response(_, 1, "disconnect", %{}))
+      assert_receive({:EXIT, ^server, {:exit_code, 0}})
+      Process.flag(:trap_exit, false)
+    end
+
+    test "succeedes when initialized", %{server: server} do
+      Process.flag(:trap_exit, true)
+      Server.receive_packet(server, initialize_req(1, %{"clientID" => "some_id"}))
+      assert_receive(response(_, 1, "initialize", %{"supportsConfigurationDoneRequest" => true}))
+      Server.receive_packet(server, request(2, "disconnect"))
+      assert_receive(response(_, 2, "disconnect", %{}))
+      assert_receive({:EXIT, ^server, {:exit_code, 0}})
+      Process.flag(:trap_exit, false)
+    end
+  end
+
   test "basic debugging", %{server: server} do
     in_fixture(__DIR__, "mix_project", fn ->
       Server.receive_packet(server, initialize_req(1, %{}))
@@ -182,6 +245,8 @@ defmodule ElixirLS.Debugger.ServerTest do
     end
 
     test "Evaluate expression with OK result", %{server: server} do
+      Server.receive_packet(server, initialize_req(1, %{}))
+
       Server.receive_packet(
         server,
         gen_packet("1 + 2 + 3 + 4")
@@ -194,6 +259,8 @@ defmodule ElixirLS.Debugger.ServerTest do
 
     @tag :capture_log
     test "Evaluate expression with ERROR result", %{server: server} do
+      Server.receive_packet(server, initialize_req(1, %{}))
+
       Server.receive_packet(
         server,
         gen_packet("1 = 2")
@@ -207,6 +274,8 @@ defmodule ElixirLS.Debugger.ServerTest do
     end
 
     test "Evaluate expression with attempt to exit debugger process", %{server: server} do
+      Server.receive_packet(server, initialize_req(1, %{}))
+
       Server.receive_packet(
         server,
         gen_packet("Process.exit(self(), :normal)")
@@ -220,6 +289,8 @@ defmodule ElixirLS.Debugger.ServerTest do
     end
 
     test "Evaluate expression with attempt to throw debugger process", %{server: server} do
+      Server.receive_packet(server, initialize_req(1, %{}))
+
       Server.receive_packet(
         server,
         gen_packet("throw(:goodmorning_bug)")
@@ -233,6 +304,8 @@ defmodule ElixirLS.Debugger.ServerTest do
     end
 
     test "Evaluate expression which has long execution", %{server: server} do
+      Server.receive_packet(server, initialize_req(1, %{}))
+
       Server.receive_packet(
         server,
         gen_packet(":timer.sleep(10_000)")

apps/elixir_ls_debugger/test/test_helper.exs

@@ -1 +1,2 @@
+Application.put_env(:elixir_ls_debugger, :test_mode, true)
 ExUnit.start(exclude: [pending: true])

apps/elixir_ls_utils/lib/launch.ex

@@ -24,13 +24,18 @@ defmodule ElixirLS.Utils.Launch do
   end
 
   def language_server_version do
-    {:ok, vsn} = :application.get_key(:language_server, :vsn)
-    vsn
+    get_version(:language_server)
   end
 
   def debugger_version do
-    {:ok, vsn} = :application.get_key(:elixir_ls_debugger, :vsn)
-    vsn
+    get_version(:elixir_ls_debugger)
+  end
+
+  defp get_version(app) do
+    case :application.get_key(app, :vsn) do
+      {:ok, version} -> List.to_string(version)
+      :undefined -> "dev"
+    end
   end
 
   defp load_dot_config do

apps/elixir_ls_utils/lib/packet_stream.ex

@@ -14,7 +14,7 @@ defmodule ElixirLS.Utils.PacketStream do
         case read_packet(pid) do
           :eof -> {:halt, :ok}
           {:error, reason} -> {:halt, {:error, reason}}
-          packet -> {[packet], :ok}
+          {:ok, packet} -> {[packet], :ok}
         end
       end,
       fn
@@ -28,7 +28,9 @@ defmodule ElixirLS.Utils.PacketStream do
   end
 
   defp read_packet(pid) do
-    header = read_header(pid)
+    header =
+      read_header(pid)
+      |> validate_content_type
 
     case header do
       :eof -> :eof
@@ -53,20 +55,77 @@ defmodule ElixirLS.Utils.PacketStream do
         if line == "" do
           header
         else
-          [key, value] = String.split(line, ": ")
-          read_header(pid, Map.put(header, key, value))
+          case String.split(line, ": ") do
+            [key, value] ->
+              read_header(pid, Map.put(header, key, value))
+
+            _ ->
+              {:error, :invalid_header}
+          end
         end
     end
   end
 
   defp read_body(pid, header) do
-    %{"Content-Length" => content_length_str} = header
-    body = IO.binread(pid, String.to_integer(content_length_str))
+    case get_content_length(header) do
+      {:ok, content_length} ->
+        case IO.binread(pid, content_length) do
+          :eof -> :eof
+          {:error, reason} -> {:error, reason}
+          body -> JasonVendored.decode(body)
+        end
 
-    case body do
-      :eof -> :eof
-      {:error, reason} -> {:error, reason}
-      body -> JasonVendored.decode!(body)
+      other ->
+        other
+    end
+  end
+
+  def get_content_length(%{"Content-Length" => content_length_str}) do
+    case Integer.parse(content_length_str) do
+      {l, ""} when l >= 0 -> {:ok, l}
+      _ -> {:error, :invalid_content_length}
+    end
+  end
+
+  def get_content_length(_), do: {:error, :invalid_content_length}
+
+  @default_mime_type "application/vscode-jsonrpc"
+  @default_charset "utf-8"
+
+  defp get_content_type(%{"Content-Type" => content_type}) do
+    case String.split(content_type, ";") do
+      [mime_type] ->
+        {mime_type |> String.trim() |> String.downcase(), @default_charset}
+
+      [mime_type | parameters] ->
+        maybe_charset =
+          for parameter <- parameters,
+              trimmed = parameter |> String.trim(),
+              trimmed |> String.starts_with?("charset="),
+              "charset=" <> value = trimmed,
+              do: value |> String.replace("\"", "") |> String.downcase()
+
+        charset =
+          case maybe_charset |> Enum.at(0) do
+            nil -> @default_charset
+            # backwards compatibility
+            "utf8" -> @default_charset
+            other -> other
+          end
+
+        {mime_type |> String.trim() |> String.downcase(), charset}
     end
   end
+
+  defp get_content_type(%{}), do: {@default_mime_type, @default_charset}
+
+  def validate_content_type(header) when is_map(header) do
+    if get_content_type(header) == {@default_mime_type, @default_charset} do
+      header
+    else
+      {:error, :not_supported_content_type}
+    end
+  end
+
+  def validate_content_type(other), do: other
 end