This is an automated email from the ASF dual-hosted git repository. hsluoyz pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/casbin-ex.git
commit 8ce8df347e7ffa8302188684c8a957a7f609cecb Author: Yang Luo <[email protected]> AuthorDate: Thu Aug 6 23:59:21 2026 +0800 feat: add EnforcerSupervisor.stop_enforcer to isolate enforcers in async tests (#31) --- README.md | 16 ++++++ guides/test_isolation.md | 86 ++++++++++++++++++++++++++++ lib/casbin/enforcer_server.ex | 45 +++++++++++++++ lib/casbin/enforcer_supervisor.ex | 40 ++++++++++++- test/enforcer_server_test.exs | 100 +++++++++++++++++++++++++++++++++ test/persist/ecto_server_load_test.exs | 26 ++++++--- 6 files changed, 303 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 1622a47..a5a24cc 100644 --- a/README.md +++ b/README.md @@ -458,6 +458,22 @@ Key topics covered: ## Testing +### Isolating enforcers in async tests + +An enforcer name is a global handle: every test using the same name talks to the +same process and the same policies, so `async: true` tests interfere with each +other. Give each test its own enforcer name and stop it when the test is over — +see [Isolating enforcers in tests](guides/test_isolation.md). + +```elixir +setup do + ename = "acl_test_#{:erlang.unique_integer([:positive])}" + {:ok, _pid} = EnforcerSupervisor.start_enforcer(ename, @cfile) + on_exit(fn -> EnforcerSupervisor.stop_enforcer(ename) end) + {:ok, ename: ename} +end +``` + ### Using with Ecto.Adapters.SQL.Sandbox If you're using Casbin-Ex with Ecto and need to wrap operations in database transactions during testing, see our guide on [Testing with Ecto.Adapters.SQL.Sandbox and Transactions](guides/sandbox_testing.md). diff --git a/guides/test_isolation.md b/guides/test_isolation.md new file mode 100644 index 0000000..b6a3797 --- /dev/null +++ b/guides/test_isolation.md @@ -0,0 +1,86 @@ +# Isolating enforcers in tests (`async: true`) + +An enforcer is a named, long-lived process. Every caller that passes the same +name talks to the same process and therefore to the same set of policies: + +```elixir +EnforcerServer.add_policy("my_enforcer", {:p, ["alice", "blog_post", "read"]}) +``` + +That is what you want in production, where one enforcer serves the whole +application. It is not what you want in a test suite: with `async: true`, +ExUnit runs test modules concurrently, so one test adding policies to +`"my_enforcer"` and another test removing them from `"my_enforcer"` will step +on each other. Typical symptoms are policies disappearing mid-test, +`list_policies/2` returning `[]` right after a successful `add_policy/2`, and +tests that pass on their own but fail when the suite runs. + +The enforcer name is the isolation boundary. Give each test its own name and +the tests stop interfering, no matter how many run at once. + +## The pattern + +```elixir +defmodule MyApp.AclTest do + use ExUnit.Case, async: true + + alias Casbin.{EnforcerServer, EnforcerSupervisor} + + @cfile "priv/casbin/model.conf" + + setup do + # Unique per test, so concurrent tests never share an enforcer. + ename = "acl_test_#{:erlang.unique_integer([:positive])}" + + {:ok, _pid} = EnforcerSupervisor.start_enforcer(ename, @cfile) + on_exit(fn -> EnforcerSupervisor.stop_enforcer(ename) end) + + {:ok, ename: ename} + end + + test "admin can read", %{ename: ename} do + :ok = EnforcerServer.add_policy(ename, {:p, ["admin", "blog_post", "read"]}) + assert EnforcerServer.allow?(ename, ["admin", "blog_post", "read"]) + end +end +``` + +## Why `stop_enforcer/1` matters + +Enforcer state is cached in an ETS table so that a *crashed* enforcer is +restarted with the policies it had before the crash. The cache is keyed by +enforcer name and outlives the process. + +`EnforcerSupervisor.stop_enforcer/1` shuts the process down and drops that +cached state, which gives you two things: + +- **No leak.** Without it, every unique name you use adds a process and an ETS + entry that live until the VM stops. Over a large suite that adds up. +- **A clean slate.** Starting an enforcer under a name that was stopped builds + it fresh from its configuration file. Without the stop, a new enforcer under + a previously used name silently inherits the old policies and ignores the + config file you passed. + +It is safe to call whether or not an enforcer is running, so it needs no guard +in `on_exit/1`. + +## If you must share one enforcer + +Sometimes the enforcer is started once by your application supervision tree and +the name is baked into the code under test. In that case the tests touching it +cannot be async: + +```elixir +use ExUnit.Case, async: false +``` + +`async: false` only serializes against *other* modules that are also +`async: false`, so it is a real constraint, not a formality: a shared enforcer +means the whole suite must agree not to run those tests concurrently. Prefer +making the enforcer name a parameter of the code under test so each test can +supply its own. + +## Related + +- [Testing with Ecto.Adapters.SQL.Sandbox and Transactions](sandbox_testing.md) — + isolating the *database* connection an enforcer uses. diff --git a/lib/casbin/enforcer_server.ex b/lib/casbin/enforcer_server.ex index 6073199..07b778a 100644 --- a/lib/casbin/enforcer_server.ex +++ b/lib/casbin/enforcer_server.ex @@ -239,11 +239,33 @@ defmodule Casbin.EnforcerServer do GenServer.call(via_tuple(ename), {:set_persist_adapter, adapter}) end + @doc """ + Returns the pid of the enforcer process registered under `ename`, or + `nil` when no enforcer is running under that name. + """ + def whereis(ename) do + # The registry unregisters a dead process asynchronously, so a lookup + # right after a shutdown can still return the pid that just went away. + case Registry.lookup(Casbin.EnforcerRegistry, ename) do + [{pid, _}] -> if Process.alive?(pid), do: pid, else: nil + [] -> nil + end + end + + @doc """ + Returns `true` if an enforcer process is running under `ename`. + """ + def running?(ename), do: whereis(ename) !== nil + # # Server Callbacks # def init({ename, cfile}) do + # Needed so `terminate/2` runs on a supervised shutdown and can drop + # the cached state of this enforcer. + Process.flag(:trap_exit, true) + case create_new_or_lookup_enforcer(ename, cfile) do {:error, reason} -> {:stop, reason} @@ -254,6 +276,21 @@ defmodule Casbin.EnforcerServer do end end + # The ets cache exists so a crashed enforcer is restarted with the state + # it had before the crash. A deliberate shutdown is not a crash: dropping + # the entry keeps the state from leaking into the next enforcer that is + # started under the same name. + def terminate(reason, _enforcer) do + if graceful_shutdown?(reason) do + case self_name() do + nil -> :ok + ename -> :ets.delete(:enforcers_table, ename) + end + end + + :ok + end + def handle_call({:allow?, req}, _from, enforcer) do allowed = enforcer |> Enforcer.allow?(req) {:reply, allowed, enforcer} @@ -314,6 +351,7 @@ defmodule Casbin.EnforcerServer do def handle_call({:save_policies}, _from, enforcer) do new_enforcer = enforcer |> Enforcer.save_policies() + :ets.insert(:enforcers_table, {self_name(), new_enforcer}) {:reply, :ok, new_enforcer} end @@ -408,6 +446,13 @@ defmodule Casbin.EnforcerServer do Registry.keys(Casbin.EnforcerRegistry, self()) |> List.first() end + # Returns `true` when the process was stopped on purpose rather than + # having crashed. + defp graceful_shutdown?(:normal), do: true + defp graceful_shutdown?(:shutdown), do: true + defp graceful_shutdown?({:shutdown, _reason}), do: true + defp graceful_shutdown?(_reason), do: false + # Creates a new enforcer or lookups existing one in the ets table. defp create_new_or_lookup_enforcer(ename, cfile) do case :ets.lookup(:enforcers_table, ename) do diff --git a/lib/casbin/enforcer_supervisor.ex b/lib/casbin/enforcer_supervisor.ex index 2670452..5212782 100644 --- a/lib/casbin/enforcer_supervisor.ex +++ b/lib/casbin/enforcer_supervisor.ex @@ -14,15 +14,51 @@ defmodule Casbin.EnforcerSupervisor do end @doc """ - Starts a new `Enforcer` process and supervises it + Starts a new `Enforcer` process and supervises it. + + A crashed enforcer is restarted with the state it had before the crash. + An enforcer stopped with `stop_enforcer/1` is not restarted. """ def start_enforcer(ename, cfile) do child_spec = %{ id: Casbin.EnforcerServer, start: {Casbin.EnforcerServer, :start_link, [ename, cfile]}, - restart: :permanent + restart: :transient } DynamicSupervisor.start_child(__MODULE__, child_spec) end + + @doc """ + Stops the enforcer registered under `ename` and drops its state. + + Starting another enforcer under the same name afterwards builds a fresh + one from its configuration file, which makes it safe to give every test + (or tenant, or request) its own enforcer name and tear it down when done. + + Always returns `:ok`, whether or not an enforcer was running, so it can + be called from an `on_exit/1` callback without a guard. + """ + def stop_enforcer(ename) do + case Casbin.EnforcerServer.whereis(ename) do + nil -> :ok + pid -> DynamicSupervisor.terminate_child(__MODULE__, pid) + end + + # `terminate/2` already drops the entry on a clean shutdown; doing it here + # too covers the enforcer that never got there (killed on a shutdown + # timeout) and the one that was gone before this call. + :ets.delete(:enforcers_table, ename) + :ok + end + + @doc """ + Returns the names of all running enforcers. + """ + def list_enforcers do + Casbin.EnforcerRegistry + |> Registry.select([{{:"$1", :"$2", :_}, [], [{{:"$1", :"$2"}}]}]) + |> Enum.filter(fn {_ename, pid} -> Process.alive?(pid) end) + |> Enum.map(fn {ename, _pid} -> ename end) + end end diff --git a/test/enforcer_server_test.exs b/test/enforcer_server_test.exs new file mode 100644 index 0000000..284702c --- /dev/null +++ b/test/enforcer_server_test.exs @@ -0,0 +1,100 @@ +defmodule Casbin.EnforcerServerTest do + use ExUnit.Case, async: true + + alias Casbin.{EnforcerServer, EnforcerSupervisor} + + @cfile "data/acl.conf" |> Path.expand(__DIR__) + @pfile "data/acl.csv" |> Path.expand(__DIR__) + + # Every test gets its own enforcer name, so tests never share state. + setup do + ename = "enforcer_server_test_#{:erlang.unique_integer([:positive])}" + on_exit(fn -> EnforcerSupervisor.stop_enforcer(ename) end) + {:ok, ename: ename} + end + + describe "stop_enforcer/1" do + test "stops the running enforcer", %{ename: ename} do + {:ok, pid} = EnforcerSupervisor.start_enforcer(ename, @cfile) + ref = Process.monitor(pid) + + assert EnforcerServer.running?(ename) + assert :ok === EnforcerSupervisor.stop_enforcer(ename) + + assert_receive {:DOWN, ^ref, :process, ^pid, _reason} + refute EnforcerServer.running?(ename) + end + + test "is not restarted by the supervisor", %{ename: ename} do + {:ok, _pid} = EnforcerSupervisor.start_enforcer(ename, @cfile) + :ok = EnforcerSupervisor.stop_enforcer(ename) + + refute ename in EnforcerSupervisor.list_enforcers() + end + + test "drops the stored state", %{ename: ename} do + {:ok, _pid} = EnforcerSupervisor.start_enforcer(ename, @cfile) + :ok = EnforcerServer.load_policies(ename, @pfile) + assert EnforcerServer.allow?(ename, ["alice", "blog_post", "create"]) === true + + :ok = EnforcerSupervisor.stop_enforcer(ename) + + # An enforcer started under the same name is built from `@cfile` + # again instead of inheriting the policies of the previous one. + {:ok, _pid} = EnforcerSupervisor.start_enforcer(ename, @cfile) + assert EnforcerServer.list_policies(ename, %{}) === [] + assert EnforcerServer.allow?(ename, ["alice", "blog_post", "create"]) === false + end + + test "is a no-op when no enforcer is running", %{ename: ename} do + assert :ok === EnforcerSupervisor.stop_enforcer(ename) + assert :ok === EnforcerSupervisor.stop_enforcer(ename) + end + end + + describe "crash recovery" do + test "a restarted enforcer keeps the policies it had", %{ename: ename} do + {:ok, pid} = EnforcerSupervisor.start_enforcer(ename, @cfile) + :ok = EnforcerServer.add_policy(ename, {:p, ["alice", "blog_post", "read"]}) + + ref = Process.monitor(pid) + Process.exit(pid, :kill) + assert_receive {:DOWN, ^ref, :process, ^pid, :killed} + + wait_until_restarted(ename, pid) + assert EnforcerServer.allow?(ename, ["alice", "blog_post", "read"]) === true + end + end + + describe "isolation between enforcers" do + test "policies added to one enforcer are invisible to another", %{ename: ename} do + other = ename <> "_other" + on_exit(fn -> EnforcerSupervisor.stop_enforcer(other) end) + + {:ok, _pid} = EnforcerSupervisor.start_enforcer(ename, @cfile) + {:ok, _pid} = EnforcerSupervisor.start_enforcer(other, @cfile) + + :ok = EnforcerServer.add_policy(ename, {:p, ["alice", "blog_post", "read"]}) + + assert EnforcerServer.allow?(ename, ["alice", "blog_post", "read"]) === true + assert EnforcerServer.allow?(other, ["alice", "blog_post", "read"]) === false + end + end + + # The registry drops a dead enforcer asynchronously, so waiting for *a* + # process under `ename` can still hand back the pid that was just killed. + # Wait for the replacement instead. + defp wait_until_restarted(ename, old_pid, attempts \\ 50) do + case EnforcerServer.whereis(ename) do + pid when is_pid(pid) and pid !== old_pid -> + :ok + + _ when attempts === 0 -> + flunk("enforcer '#{ename}' was not restarted") + + _ -> + Process.sleep(10) + wait_until_restarted(ename, old_pid, attempts - 1) + end + end +end diff --git a/test/persist/ecto_server_load_test.exs b/test/persist/ecto_server_load_test.exs index 22c3cb4..48c15b1 100644 --- a/test/persist/ecto_server_load_test.exs +++ b/test/persist/ecto_server_load_test.exs @@ -17,14 +17,15 @@ defmodule Casbin.Persist.EctoServerLoadTest do @repo_rbac MockRbacRepo setup do - # Ensure clean state - :ets.delete_all_objects(:enforcers_table) + # Every test below starts enforcers under unique names, so there is no + # shared state to clear here. Wiping `:enforcers_table` would take out + # the enforcers of the test modules running alongside this one. :ok end describe "load_policies/1 with ACL model" do test "loads policies from EctoAdapter on startup" do - ename = "test_acl_load_#{:erlang.unique_integer([:positive])}" + ename = unique_ename("test_acl_load") # Start enforcer {:ok, _pid} = EnforcerSupervisor.start_enforcer(ename, @cfile_acl) @@ -46,7 +47,7 @@ defmodule Casbin.Persist.EctoServerLoadTest do end test "returns error when no adapter is set" do - ename = "test_acl_no_adapter_#{:erlang.unique_integer([:positive])}" + ename = unique_ename("test_acl_no_adapter") # Start enforcer without setting adapter {:ok, _pid} = EnforcerSupervisor.start_enforcer(ename, @cfile_acl) @@ -58,7 +59,7 @@ defmodule Casbin.Persist.EctoServerLoadTest do describe "load_policies/1 and load_mapping_policies/1 with RBAC model" do test "loads both policies and mapping policies from EctoAdapter" do - ename = "test_rbac_load_#{:erlang.unique_integer([:positive])}" + ename = unique_ename("test_rbac_load") # Start enforcer {:ok, _pid} = EnforcerSupervisor.start_enforcer(ename, @cfile_rbac) @@ -92,7 +93,7 @@ defmodule Casbin.Persist.EctoServerLoadTest do describe "backward compatibility" do test "load_policies/2 with file path still works" do - ename = "test_file_load_#{:erlang.unique_integer([:positive])}" + ename = unique_ename("test_file_load") pfile = "../data/acl.csv" |> Path.expand(__DIR__) # Start enforcer @@ -107,7 +108,7 @@ defmodule Casbin.Persist.EctoServerLoadTest do end test "load_mapping_policies/2 with file path still works" do - ename = "test_file_mapping_load_#{:erlang.unique_integer([:positive])}" + ename = unique_ename("test_file_mapping_load") pfile = "../data/rbac.csv" |> Path.expand(__DIR__) # Start enforcer @@ -124,7 +125,7 @@ defmodule Casbin.Persist.EctoServerLoadTest do describe "persistence workflow" do test "complete workflow: set adapter, load, modify, verify" do - ename = "test_workflow_#{:erlang.unique_integer([:positive])}" + ename = unique_ename("test_workflow") # Start enforcer {:ok, _pid} = EnforcerSupervisor.start_enforcer(ename, @cfile_acl) @@ -150,4 +151,13 @@ defmodule Casbin.Persist.EctoServerLoadTest do assert length(policies) === 1 end end + + # Returns an enforcer name unique to the running test and stops that + # enforcer once the test is over, so tests stay isolated from each other + # and enforcers do not pile up while the suite runs. + defp unique_ename(prefix) do + ename = "#{prefix}_#{:erlang.unique_integer([:positive])}" + on_exit(fn -> EnforcerSupervisor.stop_enforcer(ename) end) + ename + end end --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
