diff --git a/README.md b/README.md index 59a9ddc..213ed1e 100644 --- a/README.md +++ b/README.md @@ -75,7 +75,7 @@ In this example: ### 2. Provide generic read/write functions -You'll need to provide a `read_func/3` and a `write func/3` for actually +You'll need to provide a `read_func/3` and a `write_func/3` for actually interacting on the Modbus. In practice, these functions will likely build on a library like [Modbux](https://hexdocs.pm/modbux/readme.html) along with state stored in a GenServer (e.g. a `modbux_pid`, IP Address, etc.) to perform the read/write operations. @@ -97,8 +97,8 @@ It must return either `:ok` or `{:error, message}`. ```elixir write_func = fn object_type, starting_address, value_or_values -> - result = custom_write_logic(…) - {:ok, result} + custom_write_logic(…) + :ok end ``` diff --git a/lib/modboss.ex b/lib/modboss.ex index 3596d9b..0860e54 100644 --- a/lib/modboss.ex +++ b/lib/modboss.ex @@ -61,19 +61,12 @@ defmodule ModBoss do defaults to `true`. This option can be especially useful if you need insight into a particular value that is failing to decode as expected. * `:max_attempts` — maximum number of times each `read_func` callback will - be attempted before giving up. Defaults to `1` (no retries). Callbacks that - raise an exception or return an error tuple will trigger a retry. + be attempted before giving up. Defaults to `1` (no retries). Only + `{:error, _}` triggers a retry; exceptions are not retried. * `:telemetry_label` — an arbitrary term attached as `label` in telemetry event metadata. Useful for identifying which device or connection a request belongs to. See `ModBoss.Telemetry` for details. - > #### Callback exceptions are rescued {: .warning} - > - > If `read_func` raises an exception, ModBoss rescues it and returns - > `{:error, exception}`. This applies regardless of the `:max_attempts` - > setting. The exception and stacktrace will be available via - > [per-callback telemetry events](`ModBoss.Telemetry`). - > #### Gaps {: .info} > > Every Modbus request incurs network round-trip overhead, so fewer, larger reads @@ -433,13 +426,6 @@ defmodule ModBoss do > Each batch will contain **either a list or an individual value** based on the number of > addresses to be written—so you should be prepared for both. - > #### Callback exceptions are rescued {: .warning} - > - > If `write_func` raises an exception, ModBoss rescues it and returns - > `{:error, exception}`. This applies regardless of the `:max_attempts` - > setting. The exception and stacktrace will be available via - > [per-callback telemetry events](`ModBoss.Telemetry`). - > #### Non-atomic writes! {: .warning} > > While `ModBoss.write/4` has the _feel_ of being atomic, it's important to recognize that it @@ -451,8 +437,8 @@ defmodule ModBoss do ## Opts * `:max_attempts` — maximum number of times each `write_func` callback will - be attempted before giving up. Defaults to `1` (no retries). Callbacks that - raise an exception or return an error tuple will trigger a retry. + be attempted before giving up. Defaults to `1` (no retries). Only + `{:error, _}` triggers a retry; exceptions are not retried. * `:telemetry_label` — an arbitrary term attached as `label` in telemetry event metadata. Useful for identifying which device or connection a request belongs to. See `ModBoss.Telemetry` for details. @@ -630,14 +616,7 @@ defmodule ModBoss do defp retry(max_attempts, func) do Enum.reduce_while(1..max_attempts, nil, fn attempt, _prev -> - result = - try do - func.(attempt) - rescue - e -> {:error, e} - end - - case result do + case func.(attempt) do {:error, _} = error when attempt < max_attempts -> {:cont, error} result -> {:halt, {result, attempt}} end diff --git a/lib/modboss/telemetry.ex b/lib/modboss/telemetry.ex index 632d4dc..c4abfe7 100644 --- a/lib/modboss/telemetry.ex +++ b/lib/modboss/telemetry.ex @@ -65,6 +65,26 @@ defmodule ModBoss.Telemetry do result: term() } + ### Read Exception + # Event + [:modboss, :read, :exception] + + # Measurements + %{ + duration: integer(), + monotonic_time: integer() + } + + # Metadata + %{ + schema: module(), + names: [atom()], + label: term(), + kind: atom(), + reason: term(), + stacktrace: list() + } + ### Write Start # Event [:modboss, :write, :start] @@ -103,6 +123,26 @@ defmodule ModBoss.Telemetry do result: term() } + ### Write Exception + # Event + [:modboss, :write, :exception] + + # Measurements + %{ + duration: integer(), + monotonic_time: integer() + } + + # Metadata + %{ + schema: module(), + names: [atom()], + label: term(), + kind: atom(), + reason: term(), + stacktrace: list() + } + ## Per-callback events These events wrap each individual invocation of your `read_func` or `write_func` diff --git a/test/modboss/telemetry_test.exs b/test/modboss/telemetry_test.exs index 392b4ee..e03d899 100644 --- a/test/modboss/telemetry_test.exs +++ b/test/modboss/telemetry_test.exs @@ -40,6 +40,7 @@ defmodule ModBoss.TelemetryTest do attach_many([ [:modboss, :read, :start], [:modboss, :read, :stop], + [:modboss, :read, :exception], [:modboss, :read_callback, :start], [:modboss, :read_callback, :stop], [:modboss, :read_callback, :exception] @@ -393,81 +394,28 @@ defmodule ModBoss.TelemetryTest do assert measurements.total_attempts == 4 end - test "retries through exceptions and succeeds", %{device: device} do - set_objects(device, %{{:holding_register, 1} => 42}) - raising_read = flakify(read_func(device), fn -> raise "raised!" end, flakes: 1) - - {:ok, 42} = ModBoss.read(TestSchema, :foo, raising_read, max_attempts: 2) - - # Attempt 1: raise, callback exception span - assert_receive {:telemetry, [:modboss, :read_callback, :exception], _, meta1} - assert meta1.attempt == 1 - assert meta1.max_attempts == 2 - - # Attempt 2: success, callback stop span - assert_receive {:telemetry, [:modboss, :read_callback, :stop], _, meta2} - assert meta2.attempt == 2 - assert meta2.max_attempts == 2 - assert {:ok, _} = meta2.result - - # Outer span: normal stop, no exception - assert_receive {:telemetry, [:modboss, :read, :stop], measurements, stop_meta} - assert measurements.total_attempts == 2 - assert {:ok, _} = stop_meta.result - refute_receive {:telemetry, [:modboss, :read, :exception], _, _} - end - - test "rescued read_func raise emits callback exception and outer stop with error", %{ - device: _device - } do + test "emits exception event when read_func raises", %{device: _device} do boom_func = fn _type, _addr, _count -> raise "boom!" end - {:error, %RuntimeError{message: "boom!"}} = ModBoss.read(TestSchema, :foo, boom_func) - - # Callback-level: exception event with full metadata - assert_receive {:telemetry, [:modboss, :read_callback, :start], start_measurements, - start_metadata} - - assert is_integer(start_measurements.system_time) - assert start_metadata.schema == TestSchema - assert start_metadata.names == [:foo] - assert start_metadata.object_type == :holding_register - assert start_metadata.starting_address == 1 - assert start_metadata.address_count == 1 - assert start_metadata.attempt == 1 - assert start_metadata.max_attempts == 1 + assert_raise RuntimeError, "boom!", fn -> + ModBoss.read(TestSchema, :foo, boom_func) + end - assert_receive {:telemetry, [:modboss, :read_callback, :exception], cb_measurements, - cb_metadata} - - assert is_integer(cb_measurements.duration) - assert cb_metadata.schema == TestSchema - assert cb_metadata.names == [:foo] - assert cb_metadata.object_type == :holding_register - assert cb_metadata.starting_address == 1 - assert cb_metadata.address_count == 1 - assert cb_metadata.attempt == 1 - assert cb_metadata.max_attempts == 1 - assert cb_metadata.kind == :error - assert %RuntimeError{message: "boom!"} = cb_metadata.reason - assert is_list(cb_metadata.stacktrace) - - # Outer span: normal stop (not exception) with error result - assert_receive {:telemetry, [:modboss, :read, :stop], measurements, metadata} + assert_receive {:telemetry, [:modboss, :read_callback, :start], _, _} + assert_receive {:telemetry, [:modboss, :read_callback, :exception], measurements, metadata} assert is_integer(measurements.duration) - assert measurements.modbus_requests == 1 - assert measurements.total_attempts == 1 - assert measurements.objects_requested == 1 - assert measurements.addresses_read == 1 - assert measurements.gap_addresses_read == 0 - assert measurements.max_gap_size == 0 - assert metadata.schema == TestSchema - assert metadata.names == [:foo] - assert {:error, %RuntimeError{message: "boom!"}} = metadata.result + assert metadata.kind == :error + assert metadata.attempt == 1 + assert %RuntimeError{message: "boom!"} = metadata.reason + assert is_list(metadata.stacktrace) - refute_receive {:telemetry, [:modboss, :read, :exception], _, _} + assert_receive {:telemetry, [:modboss, :read, :start], _, _} + assert_receive {:telemetry, [:modboss, :read, :exception], measurements, metadata} + assert is_integer(measurements.duration) + assert metadata.kind == :error + assert %RuntimeError{message: "boom!"} = metadata.reason end test "does not emit events for validation errors (e.g. unknown names)", %{device: device} do @@ -500,10 +448,11 @@ defmodule ModBoss.TelemetryTest do test "includes label in exception metadata when telemetry_label is provided" do boom_func = fn _type, _addr, _count -> raise "boom!" end - {:error, %RuntimeError{}} = + assert_raise RuntimeError, "boom!", fn -> ModBoss.read(TestSchema, :foo, boom_func, telemetry_label: :my_device) + end - assert_receive {:telemetry, [:modboss, :read, :stop], _, meta} + assert_receive {:telemetry, [:modboss, :read, :exception], _, meta} assert meta.label == :my_device assert_receive {:telemetry, [:modboss, :read_callback, :exception], _, cb_meta} @@ -536,6 +485,7 @@ defmodule ModBoss.TelemetryTest do attach_many([ [:modboss, :write, :start], [:modboss, :write, :stop], + [:modboss, :write, :exception], [:modboss, :write_callback, :start], [:modboss, :write_callback, :stop], [:modboss, :write_callback, :exception] @@ -714,55 +664,28 @@ defmodule ModBoss.TelemetryTest do assert measurements.total_attempts == 2 end - test "rescued write_func raise emits callback exception and outer stop with error", %{ - device: _device - } do + test "emits exception event when write_func raises", %{device: _device} do kaboom_func = fn _type, _addr, _values -> raise "kaboom!" end - {:error, %RuntimeError{message: "kaboom!"}} = + assert_raise RuntimeError, "kaboom!", fn -> ModBoss.write(TestSchema, [baz: 1], kaboom_func) + end - # Callback-level: exception event with full metadata - assert_receive {:telemetry, [:modboss, :write_callback, :start], start_measurements, - start_metadata} - - assert is_integer(start_measurements.system_time) - assert start_metadata.schema == TestSchema - assert start_metadata.names == [:baz] - assert start_metadata.object_type == :holding_register - assert start_metadata.starting_address == 3 - assert start_metadata.address_count == 1 - assert start_metadata.attempt == 1 - assert start_metadata.max_attempts == 1 - - assert_receive {:telemetry, [:modboss, :write_callback, :exception], cb_measurements, - cb_metadata} - - assert is_integer(cb_measurements.duration) - assert cb_metadata.schema == TestSchema - assert cb_metadata.names == [:baz] - assert cb_metadata.object_type == :holding_register - assert cb_metadata.starting_address == 3 - assert cb_metadata.address_count == 1 - assert cb_metadata.attempt == 1 - assert cb_metadata.max_attempts == 1 - assert cb_metadata.kind == :error - assert %RuntimeError{message: "kaboom!"} = cb_metadata.reason - assert is_list(cb_metadata.stacktrace) - - # Outer span: normal stop (not exception) with error result - assert_receive {:telemetry, [:modboss, :write, :stop], measurements, metadata} + assert_receive {:telemetry, [:modboss, :write_callback, :start], _, _} + assert_receive {:telemetry, [:modboss, :write_callback, :exception], measurements, metadata} assert is_integer(measurements.duration) - assert measurements.modbus_requests == 1 - assert measurements.total_attempts == 1 - assert measurements.objects_requested == 1 - assert metadata.schema == TestSchema - assert metadata.names == [:baz] - assert {:error, %RuntimeError{message: "kaboom!"}} = metadata.result + assert metadata.kind == :error + assert metadata.attempt == 1 + assert %RuntimeError{message: "kaboom!"} = metadata.reason + assert is_list(metadata.stacktrace) - refute_receive {:telemetry, [:modboss, :write, :exception], _, _} + assert_receive {:telemetry, [:modboss, :write, :start], _, _} + assert_receive {:telemetry, [:modboss, :write, :exception], measurements, metadata} + assert is_integer(measurements.duration) + assert metadata.kind == :error + assert %RuntimeError{message: "kaboom!"} = metadata.reason end test "does not emit events for validation errors (e.g. unknown names)", %{device: device} do @@ -793,36 +716,14 @@ defmodule ModBoss.TelemetryTest do assert cb_stop_metadata.label == %{port: :rs485, address: 12} end - test "retries through exceptions and succeeds", %{device: device} do - raising_write = flakify(write_func(device), fn -> raise "raised!" end, flakes: 1) - - :ok = ModBoss.write(TestSchema, [baz: 99], raising_write, max_attempts: 2) - - # Attempt 1: raise, callback exception span - assert_receive {:telemetry, [:modboss, :write_callback, :exception], _, meta1} - assert meta1.attempt == 1 - assert meta1.max_attempts == 2 - - # Attempt 2: success, callback stop span - assert_receive {:telemetry, [:modboss, :write_callback, :stop], _, meta2} - assert meta2.attempt == 2 - assert meta2.max_attempts == 2 - assert meta2.result == :ok - - # Outer span: normal stop, no exception - assert_receive {:telemetry, [:modboss, :write, :stop], measurements, stop_meta} - assert measurements.total_attempts == 2 - assert stop_meta.result == :ok - refute_receive {:telemetry, [:modboss, :write, :exception], _, _} - end - test "includes label in exception metadata when telemetry_label is provided" do kaboom_func = fn _type, _addr, _values -> raise "kaboom!" end - {:error, %RuntimeError{}} = + assert_raise RuntimeError, "kaboom!", fn -> ModBoss.write(TestSchema, [baz: 1], kaboom_func, telemetry_label: :my_device) + end - assert_receive {:telemetry, [:modboss, :write, :stop], _, meta} + assert_receive {:telemetry, [:modboss, :write, :exception], _, meta} assert meta.label == :my_device assert_receive {:telemetry, [:modboss, :write_callback, :exception], _, cb_meta} diff --git a/test/modboss_test.exs b/test/modboss_test.exs index bc11d38..c1343c1 100644 --- a/test/modboss_test.exs +++ b/test/modboss_test.exs @@ -1151,20 +1151,19 @@ defmodule ModBossTest do ModBoss.read(schema, [:alpha, :bravo], flaky_read, max_attempts: 2) end - test "max_attempts retries on raise and succeeds" do - device = start_supervised!({Agent, fn -> @initial_state end}) - encode_and_set(device, FakeSchema, foo: 42) - - raising_read = flakify(read_func(device), fn -> raise "raised!" end, flakes: 2) + test "max_attempts does not retry on raise" do + call_count = start_supervised!({Agent, fn -> 0 end}) - assert {:ok, 42} = ModBoss.read(FakeSchema, :foo, raising_read, max_attempts: 3) - end + boom_func = fn _type, _addr, _count -> + Agent.update(call_count, &(&1 + 1)) + raise "boom!" + end - test "max_attempts returns error when all attempts raise" do - boom_func = fn _type, _addr, _count -> raise "boom!" end + assert_raise RuntimeError, "boom!", fn -> + ModBoss.read(FakeSchema, :foo, boom_func, max_attempts: 3) + end - assert {:error, %RuntimeError{message: "boom!"}} = - ModBoss.read(FakeSchema, :foo, boom_func, max_attempts: 2) + assert Agent.get(call_count, & &1) == 1 end test "max_attempts raises on invalid values" do @@ -1441,20 +1440,19 @@ defmodule ModBossTest do {:error, "flaky"} = ModBoss.write(FakeSchema, [baz: 99], flaky_write, max_attempts: 2) end - test "max_attempts retries on raise and succeeds" do - device = start_supervised!({Agent, fn -> @initial_state end}) - - raising_write = flakify(write_func(device), fn -> raise "raised!" end, flakes: 1) + test "max_attempts does not retry on raise" do + call_count = start_supervised!({Agent, fn -> 0 end}) - :ok = ModBoss.write(FakeSchema, [baz: 99], raising_write, max_attempts: 2) - assert %{{:holding_register, 3} => 99} = get_objects(device) - end + kaboom_func = fn _type, _addr, _values -> + Agent.update(call_count, &(&1 + 1)) + raise "kaboom!" + end - test "max_attempts returns error when all attempts raise" do - boom_func = fn _type, _addr, _values -> raise "kaboom!" end + assert_raise RuntimeError, "kaboom!", fn -> + ModBoss.write(FakeSchema, [baz: 99], kaboom_func, max_attempts: 3) + end - assert {:error, %RuntimeError{message: "kaboom!"}} = - ModBoss.write(FakeSchema, [baz: 99], boom_func, max_attempts: 2) + assert Agent.get(call_count, & &1) == 1 end test "max_attempts raises on invalid values" do