More progress on the refactoring

apps/amethyst/.credo.exs

@@ -87,7 +87,6 @@
           {Credo.Check.Readability.VariableNames, []},
           {Credo.Check.Readability.WithSingleClause, []},
           {Credo.Check.Readability.BlockPipe, []},
-          {Credo.Check.Readability.SinglePipe, []},
 
           #
           ## Refactoring Opportunities
@@ -157,6 +156,7 @@
           {Credo.Check.Readability.OnePipePerLine, []},
           {Credo.Check.Readability.SeparateAliasRequire, []},
           {Credo.Check.Readability.SingleFunctionToBlockPipe, []},
+          {Credo.Check.Readability.SinglePipe, []},
           {Credo.Check.Readability.Specs, []},
           {Credo.Check.Readability.StrictModuleLayout, []},
           {Credo.Check.Readability.WithCustomTaggedTuple, []},

apps/amethyst/lib/apps/connection_handler.ex

@@ -22,7 +22,10 @@ defmodule Amethyst.ConnectionHandler do
   alias Amethyst.Minecraft.Write
   require Logger
 
-  defstruct handler: self(),
+  @enforce_keys [:handler, :socket]
+  defstruct [
+    :handler,
+    :socket,
     version: 0,
     connection_state: Amethyst.ConnectionState.Handshake,
     game: nil,
@@ -32,6 +35,85 @@ defmodule Amethyst.ConnectionHandler do
     authenticated: false,
     position: {0, 0, 0},
     player_state: %{}
+  ]
+  @type t :: %__MODULE__{
+    handler: pid(),
+    socket: :gen_tcp.socket(),
+    version: 0 | 767,
+    connection_state: Amethyst.ConnectionState.t(),
+    game: nil | Amethyst.GameCoordinator.Game,
+    encryption_state: nil | :crypto.crypto_state(),
+    decryption_state: nil | :crypto.crypto_state(),
+    compression_threshold: nil | non_neg_integer(),
+    authenticated: boolean(),
+    position: {x :: number(), y :: number(), z :: number()},
+    player_state: map()
+  }
+
+  @type operation :: (__MODULE__.t() -> __MODULE__.t())
+  defmodule Operations do
+    @moduledoc """
+    This module includes generators for common operations
+    which the `ConnectionHandler` should perform.
+    """
+    alias Amethyst.ConnectionHandler
+
+    @spec put(key :: atom(), value :: any()) :: ConnectionHandler.operation()
+    def put(key, value) do
+      fn state ->
+        Map.put(state, key, value)
+      end
+    end
+
+    @spec put_ps(key :: any(), value :: any()) :: ConnectionHandler.operation()
+    def put_ps(key, value) do
+      fn state ->
+        Map.put(state, :player_state, Map.put(state.player_state, key, value))
+      end
+    end
+
+    @spec send_packet(%{packet_type: atom()}) :: ConnectionHandler.operation()
+    def send_packet(packet) do
+      fn state ->
+      try do
+        data = Amethyst.ConnectionState.serialize(packet, state)
+
+        container = if state.compression_threshold == nil do
+          # Packet ID is included in data
+          Write.varint(byte_size(data)) <> data
+        else
+          threshold = state.compression_threshold
+          data_length = byte_size(data)
+          if data_length >= threshold do
+            compressed = Write.varint(data_length) <> :zlib.compress(data)
+            Write.varint(byte_size(compressed)) <> compressed
+          else
+            compressed = Write.varint(0) <> data
+            Write.varint(byte_size(compressed)) <> compressed
+          end
+        end
+
+        encrypted = if state.encryption_state == nil do
+          container
+        else
+          state.encryption_state |> :crypto.crypto_update(container)
+        end
+        :gen_tcp.send(state.socket, encrypted)
+      rescue
+        e ->
+          Logger.error("Error sending packet #{inspect(packet)}: #{Exception.format(:error, e, __STACKTRACE__)}")
+      end
+      end
+    end
+
+    @spec disconnect(reason :: String.t()) :: ConnectionHandler.operation()
+    def disconnect(reason) do
+      fn state ->
+        func = Amethyst.ConnectionState.disconnect(state, reason) |> send_packet()
+        func.(state)
+      end
+    end
+  end
 
   @spec child_spec(:gen_tcp.socket()) :: Supervisor.child_spec()
   def child_spec(socket) do
@@ -41,50 +123,97 @@ defmodule Amethyst.ConnectionHandler do
     }
   end
 
-  @spec start(:gen_tcp.socket(), atom(), integer()) :: no_return()
+  @spec start(:gen_tcp.socket()) :: no_return()
   def start(socket) do
     {:ok, spawn(fn ->
       Process.set_label("ConnectionHandler for #{inspect(socket)}")
-      loop(socket, %__MODULE__{handler: self()})
+      loop(%__MODULE__{socket: socket, handler: self()})
     end)}
   end
 
-  @spec start_link(:gen_tcp.socket(), atom(), integer()) :: no_return()
+  @spec start_link(:gen_tcp.socket()) :: no_return()
   def start_link(socket) do
     {:ok, spawn_link(fn ->
       Process.set_label("ConnectionHandler for #{inspect(socket)}")
-      loop(socket, %__MODULE__{handler: self()})
+      loop(%__MODULE__{socket: socket, handler: self()})
     end)}
   end
 
   @doc """
   Informs the `handler` that a packet was received and that it should handle it.
   """
-  @spec packet(handler :: pid(), id :: pos_integer(), data :: binary())
+  @spec packet(handler :: pid(), id :: pos_integer(), data :: binary()) :: :ok
   def packet(handler, id, data) do
     send(handler, {:packet, id, data})
+    :ok
   end
 
-  @spec close(handler :: pid())
+  @doc """
+  Informs the `handler` that its client's socket closed the connection and that it
+  should proceed with cleaning it up.
+  """
+  @spec close(handler :: pid()) :: :ok
   def close(handler) do
-
+    send(handler, :closed)
+    :ok
   end
 
-  @spec loop(:gen_tcp.socket(), %__MODULE__{}) :: no_return()
-  defp loop(socket, state) do
+  @doc """
+  Runs `ops` on the handler's state. All operations in `ops` will be ran sequentially
+  and will not be interrupted by another operation.
+  """
+  @spec run(handler :: pid(), ops :: [operation()]) :: :ok
+  def run(handler, ops) do
+    send(handler, {:run, ops})
+  end
+
+  @doc """
+  Runs a function on the handler's state and returns the result. This cannot update the state,
+  this may only be used to get data.
+  """
+  @spec get(handler :: pid(), f :: (__MODULE__.t() -> item)) :: item when item: any()
+  def get(handler, f) do
+    nonce = :rand.bytes(4)
+    send(handler, {:get, self(), f, nonce})
+    receive do
+      {:respond, ret, ^nonce} -> ret
+    end
+  end
+
+  @spec run_ops([operation()], __MODULE__.t()) :: {state :: __MODULE__.t(), successes :: [boolean()]}
+  defp run_ops(ops, state) do
+    Enum.reduce(Enum.with_index(ops), {state, []}, fn {op, i}, {state, sacc} ->
+      try do
+        {op.(state), [true | sacc]}
+      rescue
+        e ->
+          Logger.error("Operation error in op #{i}, continuing regardless: #{Exception.format(:error, e, __STACKTRACE__)}")
+          {state, [false | sacc]}
+      end
+    end)
+  end
+
+  @spec loop(__MODULE__.t()) :: no_return()
+  defp loop(state) do
     receive do
       :closed ->
         # Socket is closed, handle our side of the disconnect
         Process.exit(self(), :normal)
-      {:packet, id, data} ->
-        state = Enum.reduce(handle_packet(id, data, state), state, fn op, state -> op.(state) end)
-        loop(socket, state)
+      {:packet, id, data} when is_integer(id) and id >= 0 and is_binary(data) ->
+        {state, successes} = run_ops(handle_packet(id, data, state), state)
+        if !Enum.all?(successes) do
+          Logger.error("Failed to run operations of packet #{inspect(id, base: :hex)}")
+        end
+        loop(state)
       {:run, ops} ->
-        state = Enum.reduce(ops, state, fn op, state -> op.(state) end)
-        loop(socket, state)
-      {:get, from, op} ->
-        send(from, op.(state))
-        loop(socket, state)
+        {state, successes} = run_ops(ops, state)
+        if !Enum.all?(successes) do
+          Logger.error("Failed to run requested operations.")
+        end
+        loop(state)
+      {:get, from, op, nonce} ->
+        send(from, {:respond, op.(state), nonce})
+        loop(state)
     end
   end
 
@@ -92,7 +221,6 @@ defmodule Amethyst.ConnectionHandler do
     {floor(round(x) / 16.0), floor(round(z) / 16.0)}
   end
 
-  # x, z here is chunk position
   defp visible_chunks_from(x, z, view_distance) do
     (x - view_distance - 3 .. x + view_distance + 3) |> Enum.flat_map(fn ix ->
       (z - view_distance - 3 .. z + view_distance + 3) |> Enum.map(fn iz ->
@@ -212,73 +340,21 @@ defmodule Amethyst.ConnectionHandler do
     end)
   end
 
-  @spec handle_packet(pos_integer(), binary(), %__MODULE__{}) :: [(%__MODULE__{} -> %__MODULE__{})]
-  defp handle_packet(id, data, connstate, version, state) do
+  @spec handle_packet(non_neg_integer(), binary(), __MODULE__.t()) :: [operation()]
+  defp handle_packet(id, data, state) when is_struct(state, __MODULE__) do
     try do
-      packet = connstate.deserialize(id, version, data)
-      case connstate.handle(packet, version, state) do
-        :ok -> state
-        {:error, reason} ->
-          Logger.error("Error handling packet with ID #{inspect(id, base: :hex)} in state #{connstate}: #{reason}")
-          send(self(), {:disconnect, "§cError handling packet #{inspect(id, base: :hex)}:\n#{reason}"})
-          state
-        newstate ->
-          if is_map(newstate) do
-            newstate
-          else
-            Logger.warning("State change to #{inspect(newstate)} is not a map! Did you forget to return :ok?")
-            state
-          end
-      end
+      packet = Amethyst.ConnectionState.deserialize(state, id, data)
+      Amethyst.ConnectionState.handle(state, packet)
     rescue
       e ->
+        Logger.error("Error handling packet with ID #{inspect(id, base: :hex)}: #{Exception.format(:error, e, __STACKTRACE__)}")
         if Application.get_env(:amethyst, :release, false) do
-          send(self(), {:disconnect, "§cError handling packet #{inspect(id, base: :hex)}:\n#{Exception.format(:error, e, __STACKTRACE__)}"})
-          Logger.error("Error handling packet with ID #{inspect(id, base: :hex)} in state #{connstate}: #{Exception.format(:error, e, __STACKTRACE__)}")
+          [Operations.disconnect(
+            "§cError handling packet #{inspect(id, base: :hex)}:\n#{Exception.format(:error, e, __STACKTRACE__)}"
+            )]
         else
-          Logger.error("Error handling packet with ID #{inspect(id, base: :hex)} in state #{connstate}: #{Exception.format(:error, e, __STACKTRACE__)}")
-        end
-        state
+          []
         end
     end
-
-  defp send_packet(socket, connstate, packet, version, state) do
-    try do
-      data = connstate.serialize(packet, version)
-      container = if Map.get(state, :compression) == nil do
-        # Packet ID is included in data
-        Write.varint(byte_size(data)) <> data
-      else
-        threshold = Map.get(state, :compression, 0)
-        data_length = byte_size(data)
-        if data_length >= threshold do
-          compressed = Write.varint(data_length) <> :zlib.compress(data)
-          Write.varint(byte_size(compressed)) <> compressed
-        else
-          compressed = Write.varint(0) <> data
-          Write.varint(byte_size(compressed)) <> compressed
-        end
-      end
-      encrypted = if Map.get(state, :encryption_state) == nil do
-        container
-      else
-        Map.get(state, :encryption_state) |> :crypto.crypto_update(container)
-      end
-      :gen_tcp.send(socket, encrypted)
-    rescue
-      e ->
-        Logger.error("Error sending packet #{inspect(packet)} in state #{connstate}: #{Exception.format(:error, e, __STACKTRACE__)}")
-        send(self(), {:disconnect, "§cError sending packet #{inspect(packet)}:\n#{Exception.format(:error, e, __STACKTRACE__)}"})
-    end
-  end
-
-  defp disconnect(socket, reason, connstate, version, state) do
-    Logger.info("Disconnecting connection #{inspect(socket)}")
-    Logger.debug("Disconnecting connection #{inspect(socket)}: #{reason}")
-    case connstate.disconnect(reason) do
-      nil -> nil
-      packet -> send_packet(socket, connstate, packet, version, state)
-    end
-    :gen_tcp.close(socket)
   end
 end

apps/amethyst/lib/apps/connection_receiver.ex

@@ -34,7 +34,7 @@ defmodule Amethyst.ConnectionReceiver do
   def start(socket) do
     {:ok, spawn(fn ->
       Process.set_label("ConnectionReceiver for #{inspect(socket)}")
-      {:ok, pid} = Amethyst.ConnectionHandler.start_link(socket, Amethyst.ConnectionState.Handshake, 0)
+      {:ok, pid} = Amethyst.ConnectionHandler.start_link(socket)
       receive(socket, pid, nil, nil)
     end)}
   end
@@ -43,39 +43,33 @@ defmodule Amethyst.ConnectionReceiver do
   def start_link(socket) do
     {:ok, spawn_link(fn ->
       Process.set_label("ConnectionReceiver for #{inspect(socket)}")
-      {:ok, pid} = Amethyst.ConnectionHandler.start_link(socket, Amethyst.ConnectionState.Handshake, 0)
+      {:ok, pid} = Amethyst.ConnectionHandler.start_link(socket)
       receive(socket, pid, nil, nil)
     end)}
   end
 
   @spec receive(:gen_tcp.socket(), pid(), nil | :crypto.crypto_state(), nil | pos_integer()) :: no_return()
-  def receive(socket, sender, estate, cstate) do
+  def receive(socket, handler, estate, cstate) do
     case get_packet(socket, estate, cstate) do
-      :closed -> send(sender, :closed)
+      :closed -> Amethyst.ConnectionHandler.close(handler)
         Process.exit(self(), :normal)
       {:error, error} -> Logger.error("Error reading packet: #{error}")
-      {id, data} -> send(sender, {:packet, id, data})
+      {id, data} -> Amethyst.ConnectionHandler.packet(handler, id, data)
     end
 
     estate = if estate == nil do
-      # Ask the handler if we have encryption now
-      send(sender, {:get_encryption, self()})
-      receive do
-        nil -> nil
-        some -> some
+      Amethyst.ConnectionHandler.get(handler, fn s -> s.encryption_state end)
+    else
+      estate
     end
-    else estate end
 
     cstate = if cstate == nil do
-      # Ask the handler if we have encryption now
-      send(sender, {:get_compression, self()})
-      receive do
-        nil -> nil
-        some -> some
+      Amethyst.ConnectionHandler.get(handler, fn s -> s.compression_threshold end)
+    else
+      cstate
     end
-    else cstate end
 
-    receive(socket, sender, estate, cstate)
+    receive(socket, handler, estate, cstate)
   end
 
   def get_packet(client, estate, cstate) do

apps/amethyst/lib/states/behaviour.ex

@@ -0,0 +1,48 @@
+# Amethyst - An experimental Minecraft server written in Elixir.
+# Copyright (C) 2024 KodiCraft
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as
+# published by the Free Software Foundation, either version 3 of the
+# License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+defmodule Amethyst.ConnectionState do
+  @moduledoc """
+  This behaviour defines the behaviour that all connection states must implement.
+  """
+
+  alias Amethyst.ConnectionHandler
+  @type t :: module()
+
+  @callback serialize(packet :: %{packet_type: atom()}, state :: ConnectionHandler.t()) :: binary()
+  @spec serialize(state :: ConnectionHandler.t(), packet :: %{packet_type: atom()}) :: binary()
+  def serialize(state, packet) do
+    state.connection_state.serialize(packet, state)
+  end
+
+  @callback deserialize(id :: non_neg_integer(), data :: binary(), state :: ConnectionHandler.t()) :: %{packet_type: atom()}
+  @spec deserialize(state :: ConnectionHandler.t(), id :: pos_integer(), data :: binary()) :: %{packet_type: atom()}
+  def deserialize(state, id, data) do
+    state.connection_state.serialize(id, data, state)
+  end
+
+  @callback handle(packet :: %{packet_type: atom()}, state :: ConnectionHandler.t()) :: [ConnectionHandler.operation()]
+  @spec handle(state :: ConnectionHandler.t(), packet :: %{packet_type: atom()}) :: [ConnectionHandler.operation()]
+  def handle(state, packet) do
+    state.connection_state.handle(packet)
+  end
+
+  @callback disconnect(reason :: String.t(), state :: ConnectionHandler.t()) :: %{packet_type: atom()}
+  @spec disconnect(state :: ConnectionHandler.t(), reason :: String.t()) :: %{packet_type: atom()}
+  def disconnect(state, reason) do
+    state.connection_state.disconnect(reason)
+  end
+end

apps/amethyst/lib/states/configuration.ex

@@ -15,6 +15,7 @@
 # along with this program.  If not, see <https://www.gnu.org/licenses/>.
 
 defmodule Amethyst.ConnectionState.Configuration do
+  @behaviour Amethyst.ConnectionState
   require Amethyst.ConnectionState.Macros
   alias Amethyst.ConnectionState.Macros
 

apps/amethyst/lib/states/handhsake.ex

@@ -15,6 +15,7 @@
 # along with this program.  If not, see <https://www.gnu.org/licenses/>.
 
 defmodule Amethyst.ConnectionState.Handshake do
+  @behaviour Amethyst.ConnectionState
   require Amethyst.ConnectionState.Macros
   alias Amethyst.ConnectionState.Macros
 

apps/amethyst/lib/states/login.ex

@@ -15,6 +15,7 @@
 # along with this program.  If not, see <https://www.gnu.org/licenses/>.
 
 defmodule Amethyst.ConnectionState.Login do
+  @behaviour Amethyst.ConnectionState
   require Amethyst.ConnectionState.Macros
   alias Amethyst.ConnectionState.Macros
 

apps/amethyst/lib/states/macros.ex

@@ -23,6 +23,7 @@ defmodule Amethyst.ConnectionState.Macros do
   require Logger
   defmacro defpacket_serverbound(name, id, version, signature, where \\ true) do
     quote do
+      @impl true
       def deserialize(unquote(id), unquote(version), data) when unquote(where) do
         {packet, ""} = Amethyst.ConnectionState.Macros.read_signature(data, unquote(signature))
         packet |> Map.put(:packet_type, unquote(name))
@@ -32,6 +33,7 @@ defmodule Amethyst.ConnectionState.Macros do
 
   defmacro defpacket_clientbound(name, id, version, signature, where \\ true) do
     quote do
+      @impl true
       def serialize(%{packet_type: unquote(name)} = packet, unquote(version)) when unquote(where) do
         # Don't check types if we are in release mode
         if Application.get_env(:amethyst, :release, false) || Amethyst.ConnectionState.Macros.check_type(packet, unquote(signature)) do

apps/amethyst/lib/states/play.ex

@@ -15,6 +15,7 @@
 # along with this program.  If not, see <https://www.gnu.org/licenses/>.
 
 defmodule Amethyst.ConnectionState.Play do
+  @behaviour Amethyst.ConnectionState
   require Amethyst.ConnectionState.Macros
   alias Amethyst.ConnectionState.Macros
 

apps/amethyst/lib/states/status.ex

@@ -15,6 +15,7 @@
 # along with this program.  If not, see <https://www.gnu.org/licenses/>.
 
 defmodule Amethyst.ConnectionState.Status do
+  @behaviour Amethyst.ConnectionState
   require Amethyst.ConnectionState.Macros
   alias Amethyst.ConnectionState.Macros