Merge pull request #22 from elixir-toniq/hlclock-relax-constraints

relax `drift?` to match the HLC paper, enforce counter boundaries

1. Relax recv max drift to check only l - pt and not abs(l - pt). In the paper, maximum drift is limited to the absolute value of the difference between the logical and physical time as a corollary of the algorithm, which only requires checking when the logical clock is in the future of physical time. This allows us to recv old timestamps, which simplifies clients by allowing them to recv the timestamps of all events before acting and allowing this library to ensure that the causal order is preserved.

2. We were checking max drift on send, but that only catches errors caused by a system administrator setting the system clock to jump or, more commonly, causing an error whenever the computer running the VM sleeps.

Author: NeilMenne

lib/hlclock/server.ex

@@ -34,7 +34,7 @@ defmodule HLClock.Server do
   end
 
   def handle_call(:send_timestamp, _from, data) do
-    case Timestamp.send(data.timestamp, physical_time(), data.max_drift) do
+    case Timestamp.send(data.timestamp, physical_time()) do
       {:ok, timestamp} ->
         {:reply, {:ok, timestamp}, %{data | timestamp: timestamp}}
 
@@ -60,7 +60,7 @@ defmodule HLClock.Server do
   def handle_info(:periodic_send, data) do
     Process.send_after(self(), :periodic_send, interval(data))
 
-    case Timestamp.send(data.timestamp, physical_time(), data.max_drift) do
+    case Timestamp.send(data.timestamp, physical_time()) do
       {:ok, ts} ->
         {:noreply, %{data | timestamp: ts}}
 

lib/hlclock/timestamp.ex

@@ -10,6 +10,8 @@ defmodule HLClock.Timestamp do
   languages/representations.
   """
 
+  import Kernel, except: [send: 2]
+
   defstruct [:time, :counter, :node_id]
 
   alias __MODULE__, as: T
@@ -41,15 +43,25 @@ defmodule HLClock.Timestamp do
   Generate a single HLC Timestamp for sending to other nodes or
   local causality tracking
   """
-  def send(%{time: old_time, counter: counter, node_id: node_id}, pt, max_drift) do
+  def send(%{time: old_time, counter: counter, node_id: node_id}, pt) do
     new_time = max(old_time, pt)
     new_counter = advance_counter(old_time, counter, new_time)
 
-    with :ok <- handle_drift(old_time, new_time, max_drift) do
-      {:ok, new(new_time, new_counter, node_id)}
+    case handle_counter(new_counter) do
+      :ok -> {:ok, new(new_time, new_counter, node_id)}
+      err -> err
     end
   end
 
+  # Compatibility for older users of Timestamp that may be providing the max_drift.
+  def send(
+        %{time: old_time, counter: counter, node_id: node_id},
+        pt,
+        _max_drift
+      ) do
+    send(%{time: old_time, counter: counter, node_id: node_id}, pt)
+  end
+
   @doc """
   Given the current timestamp for this node and a provided remote timestamp,
   perform the merge of both logical time and logical counters. Returns the new
@@ -59,15 +71,9 @@ defmodule HLClock.Timestamp do
     new_time = Enum.max([physical_time, local.time, remote.time])
 
     with {:ok, node_id} <- compare_node_ids(local.node_id, remote.node_id),
-         :ok <-
-           handle_drift(
-             remote.time,
-             physical_time,
-             max_drift,
-             :remote_drift_violation
-           ),
-         :ok <- handle_drift(new_time, physical_time, max_drift),
-         new_counter <- merge_logical(new_time, local, remote) do
+         :ok <- handle_drift(remote.time, physical_time, max_drift),
+         new_counter <- merge_logical(new_time, local, remote),
+         :ok <- handle_counter(new_counter) do
       {:ok, new(new_time, new_counter, node_id)}
     end
   end
@@ -175,18 +181,18 @@ defmodule HLClock.Timestamp do
     end
   end
 
-  defp handle_drift(l, pt, max_drift, err \\ :clock_drift_violation) do
+  defp handle_drift(l, pt, max_drift) do
     cond do
       drift?(l, pt, max_drift) ->
-        {:error, err}
+        {:error, :remote_drift_violation}
 
       true ->
         :ok
     end
   end
 
   defp drift?(l, pt, max_drift) do
-    abs(l - pt) > max_drift
+    l - pt > max_drift
   end
 
   defp advance_counter(old_time, counter, new_time) do
@@ -199,6 +205,14 @@ defmodule HLClock.Timestamp do
     end
   end
 
+  defp handle_counter(counter) do
+    if counter > 0xFFFF do
+      {:error, :max_counter_violation}
+    else
+      :ok
+    end
+  end
+
   defimpl String.Chars do
     def to_string(ts) do
       logical_time =

test/hlclock/timestamp_test.exs

@@ -91,7 +91,7 @@ defmodule HLClock.TimestampTest do
     property "for a fixed physical time, logical counter incremented" do
       check all(time <- ntp_millis()) do
         t0 = Timestamp.new(time, 0)
-        {:ok, t1} = Timestamp.send(t0, 0, @max_drift)
+        {:ok, t1} = Timestamp.send(t0, 0)
         assert t0.counter == 0
         assert t1.counter == 1
         refute Timestamp.before?(t1, t0)
@@ -100,15 +100,14 @@ defmodule HLClock.TimestampTest do
 
     test "physical time can move backwards" do
       t0 = Timestamp.new(10, 0, 0)
-      {:ok, t1} = Timestamp.send(t0, 9, @max_drift)
+      {:ok, t1} = Timestamp.send(t0, 9)
       assert t0.time == t1.time
       assert t1.counter == 1
     end
 
-    test "send can fail due to excessive drift" do
-      t0 = Timestamp.new(0, 0, 0)
-      {:error, err} = Timestamp.send(t0, 5 + 1, @max_drift)
-      assert err == :clock_drift_violation
+    test "max counter exception" do
+      t0 = Timestamp.new(10, 0xFFFF, 0)
+      assert {:error, :max_counter_violation} == Timestamp.send(t0, 9)
     end
   end
 
@@ -122,6 +121,15 @@ defmodule HLClock.TimestampTest do
       assert t2.node_id == 0
     end
 
+    test "max counter exception" do
+      t0 = Timestamp.new(10, 0xFFFF, 0)
+      remote = Timestamp.new(9, 0xFFFF, 1)
+      pt = 9
+
+      assert {:error, :max_counter_violation} ==
+               Timestamp.recv(t0, remote, pt, @max_drift)
+    end
+
     test "events test" do
       events = [
         # valid steps