Improve crash handling during WebSock.init/1 calls (#20)

* Handle errors during websocket init

* Add tests to cover existing error handling behaviour for websock callbacks

* Credo bump to fix CI warning

Author: mtrudel

lib/websock_adapter/cowboy_adapter.ex

@@ -74,6 +74,12 @@ if Code.ensure_loaded?(:cowboy_websocket) do
       end
     end
 
+    # If websocket_init (or the handler's init) raises an error, state may still
+    # be the 3-element tuple that we get at initialization time. Since we have
+    # not yet initialized the websock at this point, just terminate and let
+    # Cowboy do its usual logging
+    def terminate(_reason, _req, {_handler, _process_flags, _state}), do: :ok
+
     defp handle_reply({:ok, state}, handler), do: {:ok, {handler, state}}
     defp handle_reply({:push, data, state}, handler), do: {:reply, data, {handler, state}}
 

mix.lock

@@ -1,17 +1,17 @@
 %{
   "bandit": {:hex, :bandit, "1.4.0", "fdf9c4b9e3a2d8579540ff90f74f514e5bec25f8cb1c7ede6fddd409509e5b4b", [:mix], [{:hpax, "~> 0.1.1", [hex: :hpax, repo: "hexpm", optional: false]}, {:plug, "~> 1.14", [hex: :plug, repo: "hexpm", optional: false]}, {:telemetry, "~> 0.4 or ~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}, {:thousand_island, "~> 1.0", [hex: :thousand_island, repo: "hexpm", optional: false]}, {:websock, "~> 0.5", [hex: :websock, repo: "hexpm", optional: false]}], "hexpm", "2d068334fe7a4ea17161b875aa112bfa7d62060e8eefb1a1117b2ab6a817e04f"},
-  "bunt": {:hex, :bunt, "0.2.1", "e2d4792f7bc0ced7583ab54922808919518d0e57ee162901a16a1b6664ef3b14", [:mix], [], "hexpm", "a330bfb4245239787b15005e66ae6845c9cd524a288f0d141c148b02603777a5"},
+  "bunt": {:hex, :bunt, "1.0.0", "081c2c665f086849e6d57900292b3a161727ab40431219529f13c4ddcf3e7a44", [:mix], [], "hexpm", "dc5f86aa08a5f6fa6b8096f0735c4e76d54ae5c9fa2c143e5a1fc7c1cd9bb6b5"},
   "cowboy": {:hex, :cowboy, "2.9.0", "865dd8b6607e14cf03282e10e934023a1bd8be6f6bacf921a7e2a96d800cd452", [:make, :rebar3], [{:cowlib, "2.11.0", [hex: :cowlib, repo: "hexpm", optional: false]}, {:ranch, "1.8.0", [hex: :ranch, repo: "hexpm", optional: false]}], "hexpm", "2c729f934b4e1aa149aff882f57c6372c15399a20d54f65c8d67bef583021bde"},
   "cowboy_telemetry": {:hex, :cowboy_telemetry, "0.4.0", "f239f68b588efa7707abce16a84d0d2acf3a0f50571f8bb7f56a15865aae820c", [:rebar3], [{:cowboy, "~> 2.7", [hex: :cowboy, repo: "hexpm", optional: false]}, {:telemetry, "~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "7d98bac1ee4565d31b62d59f8823dfd8356a169e7fcbb83831b8a5397404c9de"},
   "cowlib": {:hex, :cowlib, "2.11.0", "0b9ff9c346629256c42ebe1eeb769a83c6cb771a6ee5960bd110ab0b9b872063", [:make, :rebar3], [], "hexpm", "2b3e9da0b21c4565751a6d4901c20d1b4cc25cbb7fd50d91d2ab6dd287bc86a9"},
-  "credo": {:hex, :credo, "1.6.7", "323f5734350fd23a456f2688b9430e7d517afb313fbd38671b8a4449798a7854", [:mix], [{:bunt, "~> 0.2.1", [hex: :bunt, repo: "hexpm", optional: false]}, {:file_system, "~> 0.2.8", [hex: :file_system, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}], "hexpm", "41e110bfb007f7eda7f897c10bf019ceab9a0b269ce79f015d54b0dcf4fc7dd3"},
+  "credo": {:hex, :credo, "1.7.8", "9722ba1681e973025908d542ec3d95db5f9c549251ba5b028e251ad8c24ab8c5", [:mix], [{:bunt, "~> 0.2.1 or ~> 1.0", [hex: :bunt, repo: "hexpm", optional: false]}, {:file_system, "~> 0.2 or ~> 1.0", [hex: :file_system, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}], "hexpm", "cb9e87cc64f152f3ed1c6e325e7b894dea8f5ef2e41123bd864e3cd5ceb44968"},
   "dialyxir": {:hex, :dialyxir, "1.3.0", "fd1672f0922b7648ff9ce7b1b26fcf0ef56dda964a459892ad15f6b4410b5284", [:mix], [{:erlex, ">= 0.2.6", [hex: :erlex, repo: "hexpm", optional: false]}], "hexpm", "00b2a4bcd6aa8db9dcb0b38c1225b7277dca9bc370b6438715667071a304696f"},
   "earmark_parser": {:hex, :earmark_parser, "1.4.29", "149d50dcb3a93d9f3d6f3ecf18c918fb5a2d3c001b5d3305c926cddfbd33355b", [:mix], [], "hexpm", "4902af1b3eb139016aed210888748db8070b8125c2342ce3dcae4f38dcc63503"},
   "erlex": {:hex, :erlex, "0.2.6", "c7987d15e899c7a2f34f5420d2a2ea0d659682c06ac607572df55a43753aa12e", [:mix], [], "hexpm", "2ed2e25711feb44d52b17d2780eabf998452f6efda104877a3881c2f8c0c0c75"},
   "ex_doc": {:hex, :ex_doc, "0.29.0", "4a1cb903ce746aceef9c1f9ae8a6c12b742a5461e6959b9d3b24d813ffbea146", [:mix], [{:earmark_parser, "~> 1.4.19", [hex: :earmark_parser, repo: "hexpm", optional: false]}, {:makeup_elixir, "~> 0.14", [hex: :makeup_elixir, repo: "hexpm", optional: false]}, {:makeup_erlang, "~> 0.1", [hex: :makeup_erlang, repo: "hexpm", optional: false]}], "hexpm", "f096adb8bbca677d35d278223361c7792d496b3fc0d0224c9d4bc2f651af5db1"},
-  "file_system": {:hex, :file_system, "0.2.10", "fb082005a9cd1711c05b5248710f8826b02d7d1784e7c3451f9c1231d4fc162d", [:mix], [], "hexpm", "41195edbfb562a593726eda3b3e8b103a309b733ad25f3d642ba49696bf715dc"},
+  "file_system": {:hex, :file_system, "1.0.1", "79e8ceaddb0416f8b8cd02a0127bdbababe7bf4a23d2a395b983c1f8b3f73edd", [:mix], [], "hexpm", "4414d1f38863ddf9120720cd976fce5bdde8e91d8283353f0e31850fa89feb9e"},
   "hpax": {:hex, :hpax, "0.1.2", "09a75600d9d8bbd064cdd741f21fc06fc1f4cf3d0fcc335e5aa19be1a7235c84", [:mix], [], "hexpm", "2c87843d5a23f5f16748ebe77969880e29809580efdaccd615cd3bed628a8c13"},
-  "jason": {:hex, :jason, "1.4.0", "e855647bc964a44e2f67df589ccf49105ae039d4179db7f6271dfd3843dc27e6", [:mix], [{:decimal, "~> 1.0 or ~> 2.0", [hex: :decimal, repo: "hexpm", optional: true]}], "hexpm", "79a3791085b2a0f743ca04cec0f7be26443738779d09302e01318f97bdb82121"},
+  "jason": {:hex, :jason, "1.4.4", "b9226785a9aa77b6857ca22832cffa5d5011a667207eb2a0ad56adb5db443b8a", [:mix], [{:decimal, "~> 1.0 or ~> 2.0", [hex: :decimal, repo: "hexpm", optional: true]}], "hexpm", "c5eb0cab91f094599f94d55bc63409236a8ec69a21a67814529e8d5f6cc90b3b"},
   "makeup": {:hex, :makeup, "1.1.0", "6b67c8bc2882a6b6a445859952a602afc1a41c2e08379ca057c0f525366fc3ca", [:mix], [{:nimble_parsec, "~> 1.2.2 or ~> 1.3", [hex: :nimble_parsec, repo: "hexpm", optional: false]}], "hexpm", "0a45ed501f4a8897f580eabf99a2e5234ea3e75a4373c8a52824f6e873be57a6"},
   "makeup_elixir": {:hex, :makeup_elixir, "0.16.1", "cc9e3ca312f1cfeccc572b37a09980287e243648108384b97ff2b76e505c3555", [:mix], [{:makeup, "~> 1.0", [hex: :makeup, repo: "hexpm", optional: false]}, {:nimble_parsec, "~> 1.2.3 or ~> 1.3", [hex: :nimble_parsec, repo: "hexpm", optional: false]}], "hexpm", "e127a341ad1b209bd80f7bd1620a15693a9908ed780c3b763bccf7d200c767c6"},
   "makeup_erlang": {:hex, :makeup_erlang, "0.1.1", "3fcb7f09eb9d98dc4d208f49cc955a34218fc41ff6b84df7c75b3e6e533cc65f", [:mix], [{:makeup, "~> 1.0", [hex: :makeup, repo: "hexpm", optional: false]}], "hexpm", "174d0809e98a4ef0b3309256cbf97101c6ec01c4ab0b23e926a9e17df2077cbb"},

test/websock_adapter/cowboy_adapter_test.exs

@@ -257,6 +257,28 @@ defmodule WebSockAdapterCowboyAdapterTest do
       assert recv_connection_close_frame(client) == {:ok, <<5555::16, "BOOM"::binary>>}
       assert connection_closed_for_reading?(client)
     end
+
+    defmodule InitErrorWebSock do
+      use NoopWebSock
+
+      def init(_opts), do: raise("boom")
+    end
+
+    test "crashes are handled in a manner that Cowboy expects", context do
+      client = tcp_client(context)
+
+      warnings =
+        capture_log(fn ->
+          http1_handshake(client, InitErrorWebSock)
+
+          assert recv_connection_close_frame(client) == {:ok, <<1011::16>>}
+
+          # Give our adapter a chance to explode if it's going to
+          Process.sleep(100)
+        end)
+
+      assert warnings =~ "%RuntimeError{message: \"boom\"}"
+    end
   end
 
   describe "handle_in" do
@@ -523,6 +545,30 @@ defmodule WebSockAdapterCowboyAdapterTest do
       assert recv_connection_close_frame(client) == {:ok, <<5555::16, "BOOM"::binary>>}
       assert connection_closed_for_reading?(client)
     end
+
+    defmodule HandleInErrorWebSock do
+      use NoopWebSock
+
+      def handle_in(_data, _state), do: raise("boom")
+    end
+
+    test "crashes are handled in a manner that Cowboy expects", context do
+      client = tcp_client(context)
+
+      http1_handshake(client, HandleInErrorWebSock)
+
+      warnings =
+        capture_log(fn ->
+          send_text_frame(client, "OK")
+
+          assert recv_connection_close_frame(client) == {:ok, <<1011::16>>}
+
+          # Give our adapter a chance to explode if it's going to
+          Process.sleep(100)
+        end)
+
+      assert warnings =~ "%RuntimeError{message: \"boom\"}"
+    end
   end
 
   describe "handle_control" do
@@ -825,6 +871,31 @@ defmodule WebSockAdapterCowboyAdapterTest do
       assert recv_connection_close_frame(client) == {:ok, <<5555::16, "BOOM"::binary>>}
       assert connection_closed_for_reading?(client)
     end
+
+    defmodule HandleControlErrorWebSock do
+      use NoopWebSock
+
+      def handle_control(_data, _state), do: raise("boom")
+    end
+
+    test "crashes are handled in a manner that Cowboy expects", context do
+      client = tcp_client(context)
+
+      http1_handshake(client, HandleControlErrorWebSock)
+
+      warnings =
+        capture_log(fn ->
+          send_ping_frame(client, "OK")
+          _ = recv_pong_frame(client)
+
+          assert recv_connection_close_frame(client) == {:ok, <<1011::16>>}
+
+          # Give our adapter a chance to explode if it's going to
+          Process.sleep(100)
+        end)
+
+      assert warnings =~ "%RuntimeError{message: \"boom\"}"
+    end
   end
 
   describe "handle_info" do
@@ -1102,6 +1173,37 @@ defmodule WebSockAdapterCowboyAdapterTest do
       assert recv_connection_close_frame(client) == {:ok, <<5555::16, "BOOM"::binary>>}
       assert connection_closed_for_reading?(client)
     end
+
+    defmodule HandleInfoErrorWebSock do
+      use NoopWebSock
+
+      def handle_in(_data, state), do: {:push, {:text, :erlang.pid_to_list(self())}, state}
+
+      def handle_info(_data, _state),
+        do: raise("boom")
+    end
+
+    test "crashes are handled in a manner that Cowboy expects", context do
+      client = tcp_client(context)
+
+      http1_handshake(client, HandleInfoErrorWebSock)
+
+      send_text_frame(client, "whoami")
+      {:ok, pid} = recv_text_frame(client)
+      pid = pid |> String.to_charlist() |> :erlang.list_to_pid()
+
+      warnings =
+        capture_log(fn ->
+          Process.send(pid, "OK", [])
+
+          assert recv_connection_close_frame(client) == {:ok, <<1011::16>>}
+
+          # Give our adapter a chance to explode if it's going to
+          Process.sleep(100)
+        end)
+
+      assert warnings =~ "%RuntimeError{message: \"boom\"}"
+    end
   end
 
   describe "terminate" do