Repository: general-CbIC/poolex
Pull request #61 (2023-08-17 08:11:49)

Merge `run` interfaces
Improvements based on review comments from #59

`Poolex.run!/3` was removed in favor of `Poolex.run/3`. The new unified function returns `{:ok, result}` or `{:error, :checkout_timeout}` and not handles runtime errors anymore.
  - Reason: We should not catch errors in the caller process. The caller process itself must choose how to handle exceptions and exit signals.


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 0ea47a3..cf723b8 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -22,10 +22,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
     Poolex.run(:my_awesome_pool, fn worker -> some_work(worker) end, checkout_timeout: 10_000)
     ```
 
-- `Poolex.run/3` returns tuple `{:error, :checkout_timeout}` instead of `:all_workers_are_busy`. Also `{:runtime_error, reason}` moved to `{:error, {:runtime_error, reason}}`.
+- `Poolex.run/3` returns tuple `{:error, :checkout_timeout}` instead of `:all_workers_are_busy`.
   - Reason: It is easier to understand the uniform format of the response from the function: `{:ok, result}` or `{:error, reason}`.
 - `Poolex.caller()` type replaced with struct defined in `Poolex.Caller.t()`.
-  - Reason: We need to save uniq caller reference.
+  - Reason: We need to save unique caller references.
+- `Poolex.run!/3` was removed in favor of `Poolex.run/3`. The new unified function returns `{:ok, result}` or `{:error, :checkout_timeout}` and not handles runtime errors anymore.
+  - Reason: We should not catch errors in the caller process. The caller process itself must choose how to handle exceptions and exit signals.
 
 ### Fixed
 
diff --git a/docs/guides/example-of-use.cheatmd b/docs/guides/example-of-use.cheatmd
index 1974329..1df51ad 100644
--- a/docs/guides/example-of-use.cheatmd
+++ b/docs/guides/example-of-use.cheatmd
@@ -65,7 +65,7 @@ defmodule PoolexExample.Test do
 
   defp async_call_square_root(i) do
     Task.async(fn ->
-      Poolex.run!(
+      Poolex.run(
         :worker_pool,
         fn pid ->
           # Let's wrap the genserver call in a try-catch block. This allows us to trap any exceptions
diff --git a/docs/guides/getting-started.cheatmd b/docs/guides/getting-started.cheatmd
index 182d494..28e24d5 100644
--- a/docs/guides/getting-started.cheatmd
+++ b/docs/guides/getting-started.cheatmd
@@ -40,7 +40,7 @@ The second argument should contain a set of options for starting the pool.
 
 ## Working with the pool
 
-After the pool is initialized, you can get a free worker and perform any operations on it. This is done through the main interfaces `run/3` and `run!/3`. The functions work the same and the only difference between them is that `run/3` takes care of the runtime error handling.
+After the pool is initialized, you can get a free worker and perform any operations on it. This is done through the main interface `run/3`.
 
 The first argument is the name of the pool mentioned above.
 
@@ -52,8 +52,6 @@ The third argument contains run options. Currently, there is only one `checkout_
 iex> Poolex.start_link(pool_id: :agent_pool, worker_module: Agent, worker_args: [fn -> 5 end], workers_count: 1)
 iex> Poolex.run(:agent_pool, fn pid -> Agent.get(pid, &(&1)) end)
 {:ok, 5}
-iex> Poolex.run!(:agent_pool, fn pid -> Agent.get(pid, &(&1)) end)
-5
 ```
 
 If you would like to see examples of using Poolex, then check out [Example of Use](https://hexdocs.pm/poolex/example-of-use.html).
diff --git a/docs/guides/migration-from-poolboy.cheatmd b/docs/guides/migration-from-poolboy.cheatmd
index f9b8e77..964dd65 100644
--- a/docs/guides/migration-from-poolboy.cheatmd
+++ b/docs/guides/migration-from-poolboy.cheatmd
@@ -55,8 +55,8 @@ end
 
 ### III. Update call site
 
-Use `run!/3` to leave the same behavior. 
-If you want a safe interface with error handling, then use `run/3`.
+Use `run/3`.
+Be careful, unlike `:poolboy.transaction`, `Poolex.run` returns `{:ok, result}`.
 
 ```diff
 -  :poolboy.transaction(
@@ -64,7 +64,7 @@ If you want a safe interface with error handling, then use `run/3`.
 -    fn pid -> some_function(pid) end,
 -    :timer.seconds(10)
 -  )
-+  Poolex.run!(
++  Poolex.run(
 +    :some_pool,
 +    fn pid -> some_function(pid) end,
 +    checkout_timeout: :timer.seconds(10)
diff --git a/docs/guides/workers-and-callers-implementations.md b/docs/guides/workers-and-callers-implementations.md
index 0662d76..29907c5 100644
--- a/docs/guides/workers-and-callers-implementations.md
+++ b/docs/guides/workers-and-callers-implementations.md
@@ -4,7 +4,7 @@
 
 ## Callers
 
-`Callers` are processes that have requested to get a worker (used `run/3` or `run!/3`). Each pool keeps the information about `callers` to distribute workers to them when they are free.
+`Callers` are processes that have requested to get a worker (used `run/3`). Each pool keeps the information about `callers` to distribute workers to them when they are free.
 
 > ### Caller's typespec {: .warning}
 >
diff --git a/lib/poolex.ex b/lib/poolex.ex
index ee751a8..ccffa6c 100644
--- a/lib/poolex.ex
+++ b/lib/poolex.ex
@@ -27,7 +27,6 @@ defmodule Poolex do
 
   use GenServer, shutdown: :infinity
 
-  alias Poolex.CheckoutTimeoutError
   alias Poolex.Private.BusyWorkers
   alias Poolex.Private.DebugInfo
   alias Poolex.Private.IdleWorkers
@@ -148,68 +147,56 @@ defmodule Poolex do
   end
 
   @doc """
-  Same as `run!/3` but handles runtime_errors.
+  The main function for working with the pool.
+
+  It takes a pool identifier, a function that takes a worker process id as an argument and returns any value.
+  When executed, an attempt is made to find a free worker with specified timeout (5 seconds by default).
+  You can set the timeout using the `checkout_timeout` option.
 
   Returns:
-    * `{:error, {:runtime_error, reason}}` on errors.
+    * `{:ok, result}` if the worker was found and the function was executed successfully.
     * `{:error, :checkout_timeout}` if no free worker was found before the timeout.
 
-  See `run!/3` for more information.
-
   ## Examples
 
       iex> Poolex.start_link(pool_id: :some_pool, worker_module: Agent, worker_args: [fn -> 5 end], workers_count: 1)
-      iex> Poolex.run(:some_pool, fn _pid -> raise RuntimeError end)
-      {:error, {:runtime_error, %RuntimeError{message: "runtime error"}}}
       iex> Poolex.run(:some_pool, fn pid -> Agent.get(pid, &(&1)) end)
       {:ok, 5}
   """
   @spec run(pool_id(), (worker :: pid() -> any()), list(run_option())) ::
-          {:ok, any()} | {:error, :checkout_timeout | {:runtime_error, any()}}
+          {:ok, any()} | {:error, :checkout_timeout}
   def run(pool_id, fun, options \\ []) do
-    {:ok, run!(pool_id, fun, options)}
-  rescue
-    CheckoutTimeoutError -> {:error, :checkout_timeout}
-    runtime_error -> {:error, {:runtime_error, runtime_error}}
-  catch
-    :exit, reason -> {:error, {:runtime_error, reason}}
-  end
-
-  @doc """
-  The main function for working with the pool.
-
-  When executed, an attempt is made to obtain a worker with the specified timeout (5 seconds by default).
-  In case of successful execution of the passed function, the result will be returned, otherwise an error will be raised.
+    checkout_timeout = Keyword.get(options, :checkout_timeout, @default_checkout_timeout)
 
-  ## Examples
+    case get_idle_worker(pool_id, checkout_timeout) do
+      {:ok, worker_pid} ->
+        monitor_process = monitor_caller(pool_id, self(), worker_pid)
 
-      iex> Poolex.start_link(pool_id: :some_pool, worker_module: Agent, worker_args: [fn -> 5 end], workers_count: 1)
-      iex> Poolex.run!(:some_pool, fn pid -> Agent.get(pid, &(&1)) end)
-      5
-  """
-  @spec run!(pool_id(), (worker :: pid() -> any()), list(run_option())) :: any()
-  def run!(pool_id, fun, options \\ []) do
-    checkout_timeout = Keyword.get(options, :checkout_timeout, @default_checkout_timeout)
-    {:ok, worker_pid} = get_idle_worker(pool_id, checkout_timeout)
-    monitor_process = monitor_caller(pool_id, self(), worker_pid)
+        try do
+          {:ok, fun.(worker_pid)}
+        after
+          Process.exit(monitor_process, :kill)
+          GenServer.cast(pool_id, {:release_busy_worker, worker_pid})
+        end
 
-    try do
-      fun.(worker_pid)
-    after
-      Process.exit(monitor_process, :kill)
-      GenServer.cast(pool_id, {:release_busy_worker, worker_pid})
+      {:error, :checkout_timeout} ->
+        {:error, :checkout_timeout}
     end
   end
 
-  @spec get_idle_worker(pool_id(), timeout()) :: {:ok, worker()}
+  @spec get_idle_worker(pool_id(), timeout()) :: {:ok, worker()} | {:error, :checkout_timeout}
   defp get_idle_worker(pool_id, checkout_timeout) do
     caller_reference = make_ref()
-    GenServer.call(pool_id, {:get_idle_worker, caller_reference}, checkout_timeout)
-  catch
-    :exit,
-    {:timeout, {GenServer, :call, [_pool_id, {:get_idle_worker, caller_reference}, _timeout]}} ->
+
+    try do
+      GenServer.call(pool_id, {:get_idle_worker, caller_reference}, checkout_timeout)
+    catch
+      :exit,
+      {:timeout, {GenServer, :call, [_pool_id, {:get_idle_worker, ^caller_reference}, _timeout]}} ->
+        {:error, :checkout_timeout}
+    after
       GenServer.cast(pool_id, {:cancel_waiting, caller_reference})
-      raise CheckoutTimeoutError
+    end
   end
 
   @doc """
diff --git a/lib/poolex/checkout_timeout_error.ex b/lib/poolex/checkout_timeout_error.ex
deleted file mode 100644
index e55e48e..0000000
--- a/lib/poolex/checkout_timeout_error.ex
+++ /dev/null
@@ -1,7 +0,0 @@
-defmodule Poolex.CheckoutTimeoutError do
-  @moduledoc """
-  Raised on using `Poolex.run!/3` when a checkout times out.
-  """
-
-  defexception message: "checkout timeout"
-end


Test patch:
diff --git a/test/poolex_test.exs b/test/poolex_test.exs
index a8ca2e9..dd7fab2 100644
--- a/test/poolex_test.exs
+++ b/test/poolex_test.exs
@@ -105,9 +105,6 @@ defmodule PoolexTest do
 
       result = Poolex.run(pool_name, fn pid -> GenServer.call(pid, :do_some_work) end)
       assert result == {:ok, :some_result}
-
-      result = Poolex.run!(pool_name, fn pid -> GenServer.call(pid, :do_some_work) end)
-      assert result == :some_result
     end
 
     test "test waiting queue" do
@@ -117,7 +114,7 @@ defmodule PoolexTest do
         1..20
         |> Enum.map(fn _ ->
           Task.async(fn ->
-            Poolex.run!(pool_name, fn pid ->
+            Poolex.run(pool_name, fn pid ->
               GenServer.call(pid, {:do_some_work_with_delay, 100})
             end)
           end)
@@ -125,7 +122,7 @@ defmodule PoolexTest do
         |> Enum.map(&Task.await/1)
 
       assert length(result) == 20
-      assert Enum.all?(result, &(&1 == :some_result))
+      assert Enum.all?(result, &(&1 == {:ok, :some_result}))
     end
   end
 
@@ -293,7 +290,8 @@ defmodule PoolexTest do
 
     test "runtime errors" do
       pool_name = start_pool(worker_module: SomeWorker, workers_count: 1)
-      Poolex.run(pool_name, fn pid -> GenServer.call(pid, :do_raise) end)
+
+      catch_exit(Poolex.run(pool_name, fn pid -> GenServer.call(pid, :do_raise) end))
 
       :timer.sleep(10)
 
@@ -359,25 +357,7 @@ defmodule PoolexTest do
       assert debug_info.idle_workers_count == 0
     end
 
-    test "run!/3 raises an error on checkout timeout" do
-      pool_name = start_pool(worker_module: SomeWorker, workers_count: 1)
-      launch_long_task(pool_name)
-
-      assert catch_error(
-               Poolex.run!(
-                 pool_name,
-                 fn pid -> GenServer.call(pid, {:do_some_work_with_delay, :timer.seconds(4)}) end,
-                 checkout_timeout: 100
-               )
-             ) == %Poolex.CheckoutTimeoutError{message: "checkout timeout"}
-
-      :timer.sleep(10)
-
-      debug_info = Poolex.get_debug_info(pool_name)
-      assert Enum.empty?(debug_info.waiting_callers)
-    end
-
-    test "handle worker's timout" do
+    test "handle worker's timeout" do
       pool_name = start_pool(worker_module: SomeWorker, workers_count: 1)
       delay = 100
 
@@ -386,12 +366,12 @@ defmodule PoolexTest do
                  GenServer.call(pid, {:do_some_work_with_delay, delay}, 1000)
                end)
 
-      assert {:error,
-              {:runtime_error,
-               {:timeout, {GenServer, :call, [_pid, {:do_some_work_with_delay, ^delay}, 1]}}}} =
-               Poolex.run(pool_name, fn pid ->
-                 GenServer.call(pid, {:do_some_work_with_delay, delay}, 1)
-               end)
+      assert {:timeout, {GenServer, :call, [_worker_pid, {:do_some_work_with_delay, 100}, 1]}} =
+               catch_exit(
+                 Poolex.run(pool_name, fn pid ->
+                   GenServer.call(pid, {:do_some_work_with_delay, delay}, 1)
+                 end)
+               )
     end
 
     test "worker not hangs in busy status after checkout timeout" do
@@ -507,7 +487,7 @@ defmodule PoolexTest do
       pid = self()
 
       spawn(fn ->
-        Poolex.run!(pool_name, fn server ->
+        Poolex.run(pool_name, fn server ->
           SomeWorker.traceable_call(server, pid, :foo, 50)
         end)
       end)
@@ -554,7 +534,7 @@ defmodule PoolexTest do
       state = Poolex.get_state(pool_name)
 
       supervisor_pid = state.supervisor
-      worker_pid = Poolex.run!(pool_name, fn pid -> pid end)
+      {:ok, worker_pid} = Poolex.run(pool_name, fn pid -> pid end)
 
       pool_monitor_ref = Process.monitor(pool_pid)
       supervisor_monitor_ref = Process.monitor(supervisor_pid)
@@ -576,7 +556,7 @@ defmodule PoolexTest do
       state = Poolex.get_state(pool_name)
 
       supervisor_pid = state.supervisor
-      worker_pid = Poolex.run!(pool_name, fn pid -> pid end)
+      {:ok, worker_pid} = Poolex.run(pool_name, fn pid -> pid end)
 
       pool_monitor_ref = Process.monitor(pool_pid)
       supervisor_monitor_ref = Process.monitor(supervisor_pid)