Add test coverage to packet stream and wire protocol modules (#429)

* add comment

* validate content length

* add comment

closes #236

* add packet stream tests

* avoid binary copying by using IO data

avoid sending unneeded /r/n/r/n after body

* add wire protocol tests

* run formatter

Author: lukaszsamson

apps/elixir_ls_utils/lib/packet_stream.ex

@@ -12,9 +12,18 @@ defmodule ElixirLS.Utils.PacketStream do
       fn -> :ok end,
       fn _acc ->
         case read_packet(pid) do
-          :eof -> {:halt, :ok}
-          {:error, reason} -> {:halt, {:error, reason}}
-          {:ok, packet} -> {[packet], :ok}
+          :eof ->
+            {:halt, :ok}
+
+          {:error, reason} ->
+            # jsonrpc 2.0 requires that server responds with
+            # {"jsonrpc": "2.0", "error": {"code": -32700, "message": "Parse error"}, "id": null}
+            # when message fails to parse
+            # instead we halt on any error - it's not woth to handle faulty clients
+            {:halt, {:error, reason}}
+
+          {:ok, packet} ->
+            {[packet], :ok}
         end
       end,
       fn
@@ -70,9 +79,22 @@ defmodule ElixirLS.Utils.PacketStream do
     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)
+          :eof ->
+            :eof
+
+          {:error, reason} ->
+            {:error, reason}
+
+          body ->
+            case IO.iodata_length(body) do
+              ^content_length ->
+                # though jason docs suggest using `strings: :copy` when parts of binary may be stored
+                # processes/ets in our case it does not help (as of OTP 23)
+                JasonVendored.decode(body)
+
+              _other ->
+                {:error, :truncated}
+            end
         end
 
       other ->

apps/elixir_ls_utils/lib/wire_protocol.ex

@@ -4,16 +4,24 @@ defmodule ElixirLS.Utils.WireProtocol do
   """
   alias ElixirLS.Utils.{PacketStream, OutputDevice}
 
+  @separator "\r\n\r\n"
+
   def send(packet) do
     pid = io_dest()
-    body = JasonVendored.encode!(packet) <> "\r\n\r\n"
-
-    case IO.binwrite(pid, "Content-Length: #{byte_size(body)}\r\n\r\n" <> body) do
+    body = JasonVendored.encode_to_iodata!(packet)
+
+    case IO.binwrite(pid, [
+           "Content-Length: ",
+           IO.iodata_length(body) |> Integer.to_string(),
+           @separator,
+           body
+         ]) do
       :ok ->
         :ok
 
       {:error, reason} ->
         IO.warn("Unable to write to the device: #{inspect(reason)}")
+        :ok
     end
   end
 

apps/elixir_ls_utils/test/fixtures/protocol_messages/invalid_after_valid

@@ -0,0 +1,7 @@
+Content-Length: 16
+
+{"some":"value"}Content-Length: 16
+
+{"some":"value1"Content-Length: 16
+
+{"some":"value"}

apps/elixir_ls_utils/test/fixtures/protocol_messages/invalid_content_length

@@ -0,0 +1,3 @@
+Content-Length: 16
+
+{"some":"val

apps/elixir_ls_utils/test/fixtures/protocol_messages/invalid_content_type

@@ -0,0 +1,4 @@
+Content-Length: 16
+Content-Type: text/plain
+
+{"some":"value"}

apps/elixir_ls_utils/test/fixtures/protocol_messages/invalid_json

@@ -0,0 +1,3 @@
+Content-Length: 15
+
+{"some":"value}

apps/elixir_ls_utils/test/fixtures/protocol_messages/no_body_0

@@ -0,0 +1,2 @@
+Content-Length: 15
+

apps/elixir_ls_utils/test/fixtures/protocol_messages/no_body_1

@@ -0,0 +1 @@
+Content-Length: 15

apps/elixir_ls_utils/test/fixtures/protocol_messages/no_body_2

@@ -0,0 +1 @@
+Content-Length: 15

apps/elixir_ls_utils/test/fixtures/protocol_messages/no_body_3

@@ -0,0 +1 @@
+Content-Len