diff --git a/.credo.exs b/.credo.exs index 7b7510a..01e8e13 100644 --- a/.credo.exs +++ b/.credo.exs @@ -25,13 +25,15 @@ allowed_imports = [ "apps/*/lib/", "apps/*/src/", "apps/*/test/", - "apps/*/web/" + "apps/*/web/", + "mix.exs", + ".credo.exs" ], excluded: [~r"_build/", ~r"deps/"] }, plugins: [], strict: true, - parse_timeout: 10000, + parse_timeout: 10_000, color: true, checks: [ @@ -40,14 +42,73 @@ allowed_imports = [ {BlitzCredoChecks.SetWarningsAsErrorsInTest, false}, {BlitzCredoChecks.DocsBeforeSpecs, []}, {BlitzCredoChecks.DoctestIndent, []}, - {BlitzCredoChecks.NoAsyncFalse, []}, + # Superseded by MikaCredoRules.AsyncTrueRequired: the choice must be explicit, and a + # deliberate async: false is allowed. + {BlitzCredoChecks.NoAsyncFalse, false}, {BlitzCredoChecks.NoDSLParentheses, []}, {BlitzCredoChecks.NoIsBitstring, []}, - {BlitzCredoChecks.StrictComparison, []}, + {BlitzCredoChecks.StrictComparison, false}, {BlitzCredoChecks.UseStream, []}, {BlitzCredoChecks.LowercaseTestNames, []}, {BlitzCredoChecks.ImproperImport, allowed_modules: allowed_imports}, + # MikaCredoRules (supersedes BlitzCredoChecks.StrictComparison, TagTODO, TagFIXME) + {MikaCredoRules.AsyncTrueRequired, []}, + {MikaCredoRules.CredoConfigNamedDefault, []}, + {MikaCredoRules.DistributionRequiresBuckets, []}, + {MikaCredoRules.EnsureLoadedBeforeExported, []}, + # lib/cache/ets.ex: opts_definition/1 is a NimbleOptions :custom validator, whose contract + # is {:error, String.t()} + {MikaCredoRules.ErrorMessageRequired, [excluded_paths: ["_test.exs", "test/", "lib/cache/ets.ex"]]}, + {MikaCredoRules.ExceptionNamesEndInError, []}, + {MikaCredoRules.GenServerRequiresHandleContinue, []}, + {MikaCredoRules.LoggerModulePrefixAndInspect, []}, + {MikaCredoRules.LowercaseErrorMessages, []}, + {MikaCredoRules.NoAccessOnStructSubject, []}, + {MikaCredoRules.NoApplicationEnvOutsideConfig, []}, + {MikaCredoRules.NoAtomStringKeyFallback, []}, + {MikaCredoRules.NoBarePatternMatchOnFallible, []}, + {MikaCredoRules.NoBinaryPatternForStringPrefix, []}, + {MikaCredoRules.NoBlanketRescue, []}, + {MikaCredoRules.NoBooleanLiteralComparison, []}, + {MikaCredoRules.NoCondElseAtom, []}, + {MikaCredoRules.NoDoPrefixedHelper, []}, + {MikaCredoRules.NoForWithDiscardedResult, []}, + {MikaCredoRules.NoHardcodedSecretLiterals, []}, + {MikaCredoRules.NoIOANSIRawEscapes, []}, + {MikaCredoRules.NoIdentityRewrap, []}, + {MikaCredoRules.NoIntermediateSingleUseVariable, []}, + {MikaCredoRules.NoKernelPrefix, []}, + {MikaCredoRules.NoLengthZeroComparison, []}, + {MikaCredoRules.NoMixEnvAtRuntime, []}, + {MikaCredoRules.NoMockingLibraries, []}, + {MikaCredoRules.NoNilComparison, []}, + {MikaCredoRules.NoPipeIntoControlFlow, []}, + {MikaCredoRules.NoProcessSleepInTests, []}, + {MikaCredoRules.NoSelfSendZeroDelay, []}, + {MikaCredoRules.NoSingleLetterVariables, []}, + {MikaCredoRules.NoTaskAsyncInGenServer, []}, + {MikaCredoRules.NoTruthyAndOr, []}, + {MikaCredoRules.NoUnboundBlockAssignment, []}, + {MikaCredoRules.NoUnsupervisedTaskStart, []}, + {MikaCredoRules.NoVacuousAssert, []}, + {MikaCredoRules.NoWordSigilLists, []}, + {MikaCredoRules.RefuteOverAssertNot, []}, + {MikaCredoRules.SingleModulePerFile, + [excluded_paths: ["test/", "test/support/", "_test.exs", "cache_test_modules.ex"]]}, + {MikaCredoRules.StrictEquality, []}, + {MikaCredoRules.TaskAsyncStreamRequiresTimeout, []}, + {MikaCredoRules.TestOnlyDepsScoped, + [test_only_packages: [:credo, :blitz_credo_checks, :mika_credo_rules, :dialyxir, :excoveralls, :ex_doc]]}, + {MikaCredoRules.TodosNeedTickets, []}, + + # MikaCredoRules that target consumers of this library, not the library itself + {MikaCredoRules.CacheOptsNoHardcodedUri, false}, + {MikaCredoRules.CacheRequiresSandboxOption, false}, + {MikaCredoRules.NoDirectErlangRpc, false}, + {MikaCredoRules.NoRawEts, false}, + {MikaCredoRules.NoReimplementedHelper, false}, + # Consistency Checks {Credo.Check.Consistency.ExceptionNames, []}, {Credo.Check.Consistency.LineEndings, []}, @@ -60,8 +121,8 @@ allowed_imports = [ {Credo.Check.Design.AliasUsage, false}, # No outstanding TODOs - {Credo.Check.Design.TagTODO, []}, - {Credo.Check.Design.TagFIXME, []}, + {Credo.Check.Design.TagTODO, false}, + {Credo.Check.Design.TagFIXME, false}, # # Readability Checks {Credo.Check.Readability.AliasOrder, false}, diff --git a/CHANGELOG.md b/CHANGELOG.md index c24f65d..b63a8df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,9 @@ ## Bug Fixes +- fix(refresh_ahead, hash_ring): the background refresh task and the read-repair delete task now run under a `Task.Supervisor` inside the strategy's own supervision tree instead of an unsupervised `Task.start/1`. +- fix(hash_ring): an exception raised by the RPC module during read-repair is logged at warning level before the node is treated as unavailable, instead of being swallowed. +- fix(hash_ring): `Cache.HashRing.RingMonitor` seeds its history table and subscribes to node events from `handle_continue/2` rather than blocking in `init/1`. - fix: `:compression_level` is reachable. It was unusable on every path — no adapter declares it, so `NimbleOptions` rejected it on compile-time adapter opts, and it resolved to `nil` before it could reach the encoder otherwise. It is now an option on the `use Cache` line (`compression_level: 6`), it is taken off the adapter opts before they are validated so the `opts: [compression_level: 6]` spelling works too, and it is never handed to the adapter. Setting it forces encoding on adapters that hold terms natively — asking for compression is asking for bytes. A cache using a strategy adapter raises at compile time rather than ignoring the option. - fix: `Cache.ConCache.get_or_store/3` followed by `get/1` no longer raises. `get_or_store/3` writes through ConCache directly, bypassing the encode in `put/3`, so the matching `get/1` tried to `binary_to_term/1` a raw term. - fix: a binary value wrapped in braces but not valid JSON (eg `"{oops}"`) no longer raises `Jason.DecodeError` on read. `Cache.TermEncoder.decode/1` used `Jason.decode!/1`, and now falls back to returning the binary unchanged. diff --git a/lib/cache.ex b/lib/cache.ex index 4026a59..0ca7a2f 100644 --- a/lib/cache.ex +++ b/lib/cache.ex @@ -153,11 +153,11 @@ defmodule Cache do defp adapter_options!({module, fun, args}), do: apply(module, fun, args) match?({_, _}, @adapter_opts) -> - defp adapter_options!({app, key}), do: Application.fetch_env!(app, key) + defp adapter_options!({app, key}), do: Cache.Config.fetch_env!(app, key) is_atom(@adapter_opts) and not is_nil(@adapter_opts) -> defp adapter_options!(app_name) when is_atom(app_name), - do: Application.fetch_env!(app_name, __MODULE__) + do: Cache.Config.fetch_env!(app_name, __MODULE__) is_function(@adapter_opts, 0) -> defp adapter_options!(fun) when is_function(fun, 0), do: fun.() @@ -211,7 +211,9 @@ defmodule Cache do @cache_name |> @cache_strategy_module.get(key, @cache_strategy_config, adapter_options()) |> handle_adapter_result(:get, @cache_name) - |> case do + + result = + case result do {:ok, nil} = res -> :telemetry.execute([:elixir_cache, :cache, :get, :miss], %{count: 1}, %{ cache_name: @cache_name @@ -357,11 +359,11 @@ defmodule Cache do defp adapter_options!({module, fun, args}), do: apply(module, fun, args) match?({_, _}, @adapter_opts) -> - defp adapter_options!({app, key}), do: Application.fetch_env!(app, key) + defp adapter_options!({app, key}), do: Cache.Config.fetch_env!(app, key) is_atom(@adapter_opts) and not is_nil(@adapter_opts) -> defp adapter_options!(app_name) when is_atom(app_name), - do: Application.fetch_env!(app_name, __MODULE__) + do: Cache.Config.fetch_env!(app_name, __MODULE__) is_function(@adapter_opts, 0) -> defp adapter_options!(fun) when is_function(fun, 0), do: fun.() @@ -426,7 +428,9 @@ defmodule Cache do @cache_name |> @cache_adapter.get(key, adapter_options()) |> handle_adapter_result(:get, @cache_name) - |> case do + + result = + case result do {:ok, nil} = res -> :telemetry.execute([:elixir_cache, :cache, :get, :miss], %{count: 1}, %{ cache_name: @cache_name diff --git a/lib/cache/case_template.ex b/lib/cache/case_template.ex index 1347058..ec5e12a 100644 --- a/lib/cache/case_template.ex +++ b/lib/cache/case_template.ex @@ -118,10 +118,10 @@ defmodule Cache.CaseTemplate do |> Supervisor.which_children() |> Enum.filter(fn {_id, _pid, _type, modules} -> is_list(modules) and - Enum.any?(modules, &function_exported?(&1, :cache_name, 0)) + Enum.any?(modules, &(Code.ensure_loaded?(&1) and function_exported?(&1, :cache_name, 0))) end) |> Enum.flat_map(fn {_id, _pid, _type, modules} -> - Enum.filter(modules, &function_exported?(&1, :cache_name, 0)) + Enum.filter(modules, &(Code.ensure_loaded?(&1) and function_exported?(&1, :cache_name, 0))) end) end end diff --git a/lib/cache/con_cache.ex b/lib/cache/con_cache.ex index f498ffd..c2c3370 100644 --- a/lib/cache/con_cache.ex +++ b/lib/cache/con_cache.ex @@ -98,26 +98,26 @@ defmodule Cache.ConCache do def put(cache_name, key, _ttl \\ nil, value, _opts \\ []) def put(cache_name, key, nil, value, opts) do - if is_nil(opts[:dirty?]) or opts[:dirty?] do + if Keyword.get(opts, :dirty?, true) do ConCache.dirty_put(cache_name, key, value) else ConCache.put(cache_name, key, value) end rescue - exception -> + exception in ArgumentError -> {:error, ErrorMessage.internal_server_error(Exception.message(exception), %{cache: cache_name, key: key})} end def put(cache_name, key, ttl, value, opts) do item = %ConCache.Item{value: value, ttl: ttl} - if is_nil(opts[:dirty?]) or opts[:dirty?] do + if Keyword.get(opts, :dirty?, true) do ConCache.dirty_put(cache_name, key, item) else ConCache.put(cache_name, key, item) end rescue - exception -> + exception in ArgumentError -> {:error, ErrorMessage.internal_server_error(Exception.message(exception), %{cache: cache_name, key: key})} end @@ -132,7 +132,7 @@ defmodule Cache.ConCache do def delete(cache_name, key, _opts \\ []) do ConCache.delete(cache_name, key) rescue - exception -> + exception in ArgumentError -> {:error, ErrorMessage.internal_server_error(Exception.message(exception), %{cache: cache_name, key: key})} end diff --git a/lib/cache/config.ex b/lib/cache/config.ex index c73c30a..199496d 100644 --- a/lib/cache/config.ex +++ b/lib/cache/config.ex @@ -6,4 +6,6 @@ defmodule Cache.Config do def sandbox_sleep_ms do Application.get_env(@app, :sandbox_sleep_ms, 50) end + + def fetch_env!(app, key), do: Application.fetch_env!(app, key) end diff --git a/lib/cache/counter.ex b/lib/cache/counter.ex index 215644a..57a1c81 100644 --- a/lib/cache/counter.ex +++ b/lib/cache/counter.ex @@ -148,7 +148,7 @@ defmodule Cache.Counter do {:error, ErrorMessage.bad_request("integer key #{key} is out of bounds (initial_size: #{size})", %{cache: cache_name, key: key})} end rescue - exception -> + exception in ArgumentError -> {:error, ErrorMessage.internal_server_error(Exception.message(exception), %{cache: cache_name, key: key})} end @@ -166,7 +166,7 @@ defmodule Cache.Counter do :counters.add(ref, compute_index(ref, key), value) :ok rescue - exception -> + exception in ArgumentError -> {:error, ErrorMessage.internal_server_error(Exception.message(exception), %{cache: cache_name, key: key})} end @@ -184,7 +184,7 @@ defmodule Cache.Counter do :counters.put(ref, compute_index(ref, key), 0) :ok rescue - exception -> + exception in ArgumentError -> {:error, ErrorMessage.internal_server_error(Exception.message(exception), %{cache: cache_name, key: key})} end @@ -194,7 +194,7 @@ defmodule Cache.Counter do :counters.add(ref, compute_index(ref, key), step) :ok rescue - exception -> + exception in ArgumentError -> {:error, ErrorMessage.internal_server_error(Exception.message(exception), %{cache: cache_name, key: key})} end @@ -204,7 +204,7 @@ defmodule Cache.Counter do :counters.add(ref, compute_index(ref, key), -step) :ok rescue - exception -> + exception in ArgumentError -> {:error, ErrorMessage.internal_server_error(Exception.message(exception), %{cache: cache_name, key: key})} end diff --git a/lib/cache/dets.ex b/lib/cache/dets.ex index 2c14284..1c57b89 100644 --- a/lib/cache/dets.ex +++ b/lib/cache/dets.ex @@ -305,8 +305,8 @@ defmodule Cache.DETS do @doc """ Returns the list of objects associated with slot I. """ - def slot(i) do - :dets.slot(@cache_name, i) + def slot(index) do + :dets.slot(@cache_name, index) end @doc """ @@ -401,7 +401,10 @@ defmodule Cache.DETS do |> Keyword.drop([:table_name, :file_path]) |> Kernel.++(access: :read_write, file: file_path) - {:ok, _} = :dets.open_file(table_name, opts) + case :dets.open_file(table_name, opts) do + {:ok, _table} -> :ok + {:error, reason} -> exit({:dets_open_file, reason}) + end send(parent, {ref, :ready}) Process.hibernate(Function, :identity, [nil]) diff --git a/lib/cache/ets.ex b/lib/cache/ets.ex index 0782b70..a800e33 100644 --- a/lib/cache/ets.ex +++ b/lib/cache/ets.ex @@ -448,8 +448,8 @@ defmodule Cache.ETS do @doc """ Returns the list of objects associated with slot I. """ - def slot(i) do - @cache_adapter.slot(@cache_name, i) + def slot(index) do + @cache_adapter.slot(@cache_name, index) end @doc """ @@ -621,7 +621,7 @@ defmodule Cache.ETS do end defp maybe_compressed(original_opts, validated) do - if Keyword.has_key?(original_opts, :compressed) and validated[:compressed], do: :compressed + if Keyword.has_key?(original_opts, :compressed) && validated[:compressed], do: :compressed end defp maybe_kv(original_opts, validated, key) do @@ -673,15 +673,15 @@ defmodule Cache.ETS do if File.exists?(file_path) do case :ets.file2tab(String.to_charlist(file_path)) do {:ok, ^table_name} -> - Logger.info("[Cache.ETS] Rehydrated #{table_name} from #{file_path}") + Logger.info("#{__MODULE__}: rehydrated table #{inspect(table_name)} from #{inspect(file_path)}") :ok {:ok, _other_name} -> - Logger.warning("[Cache.ETS] File #{file_path} has different table name, creating new table #{table_name}") + Logger.warning("#{__MODULE__}: file #{inspect(file_path)} has a different table name, creating new table #{inspect(table_name)}") :ets.new(table_name, ets_opts) {:error, reason} -> - Logger.warning("[Cache.ETS] Failed to rehydrate #{table_name} from #{file_path}: #{inspect(reason)}, creating new table") + Logger.warning("#{__MODULE__}: failed to rehydrate #{inspect(table_name)} from #{inspect(file_path)}, creating new table, reason: #{inspect(reason)}") :ets.new(table_name, ets_opts) end else @@ -703,7 +703,7 @@ defmodule Cache.ETS do end) do {:ok, _ref} -> :ok {:error, reason} -> - Logger.error("[Cache.ETS] Failed to setup exit signal handler for #{table_name}: #{inspect(reason)}") + Logger.error("#{__MODULE__}: failed to setup exit signal handler for #{inspect(table_name)}, reason: #{inspect(reason)}") :ok end @@ -732,7 +732,8 @@ defmodule Cache.ETS do [] -> {:ok, nil} end rescue - e -> {:error, ErrorMessage.internal_server_error(Exception.message(e), %{cache: cache_name, key: key})} + exception in ArgumentError -> + {:error, ErrorMessage.internal_server_error(Exception.message(exception), %{cache: cache_name, key: key})} end @impl Cache @@ -742,7 +743,8 @@ defmodule Cache.ETS do :ok rescue - e -> {:error, ErrorMessage.internal_server_error(Exception.message(e), %{cache: cache_name, key: key})} + exception in ArgumentError -> + {:error, ErrorMessage.internal_server_error(Exception.message(exception), %{cache: cache_name, key: key})} end @impl Cache @@ -752,7 +754,8 @@ defmodule Cache.ETS do :ok rescue - e -> {:error, ErrorMessage.internal_server_error(Exception.message(e), %{cache: cache_name, key: key})} + exception in ArgumentError -> + {:error, ErrorMessage.internal_server_error(Exception.message(exception), %{cache: cache_name, key: key})} end # ETS-specific module-level functions called via @cache_adapter from the __using__ macro. @@ -841,7 +844,7 @@ defmodule Cache.ETS do def setopts(cache_name, opts), do: :ets.setopts(cache_name, opts) - def slot(cache_name, i), do: :ets.slot(cache_name, i) + def slot(cache_name, index), do: :ets.slot(cache_name, index) def tab2file(cache_name, filename), do: :ets.tab2file(cache_name, filename) def tab2file(cache_name, filename, options), do: :ets.tab2file(cache_name, filename, options) diff --git a/lib/cache/hash_ring.ex b/lib/cache/hash_ring.ex index 53b6443..fca17b1 100644 --- a/lib/cache/hash_ring.ex +++ b/lib/cache/hash_ring.ex @@ -78,6 +78,8 @@ defmodule Cache.HashRing do @behaviour Cache.Strategy + require Logger + @strategy_keys [:ring_opts, :node_weight, :rpc_module, :ring_history_size, :__cache_module__] @opts_definition [ @@ -143,7 +145,8 @@ defmodule Cache.HashRing do [ underlying_adapter.child_spec({cache_name, underlying_opts}), managed_ring_spec, - ring_monitor_spec + ring_monitor_spec, + {Task.Supervisor, name: task_supervisor_name(cache_name)} ], [strategy: :one_for_one] ]} @@ -250,7 +253,7 @@ defmodule Cache.HashRing do rpc.call(current_node, underlying_adapter, :put, [cache_name, key, nil, encoded, underlying_opts]) end - Task.start(fn -> + Task.Supervisor.start_child(task_supervisor_name(cache_name), fn -> rpc.call(old_node, underlying_adapter, :delete, [cache_name, key, underlying_opts]) end) end @@ -262,11 +265,16 @@ defmodule Cache.HashRing do {:badrpc, _} -> :unavailable end rescue - _ -> :unavailable + exception -> + Logger.warning("#{__MODULE__}: rpc get on #{inspect(node)} raised, error: #{inspect(exception)}") + + :unavailable catch :exit, _ -> :unavailable end + defp task_supervisor_name(cache_name), do: :"#{cache_name}_hash_ring_tasks" + defp key_to_node(cache_name, key) do ring = ring_name(cache_name) diff --git a/lib/cache/hash_ring/ring_monitor.ex b/lib/cache/hash_ring/ring_monitor.ex index 433250f..7cca764 100644 --- a/lib/cache/hash_ring/ring_monitor.ex +++ b/lib/cache/hash_ring/ring_monitor.ex @@ -35,9 +35,6 @@ defmodule Cache.HashRing.RingMonitor do node_whitelist = Keyword.get(opts, :node_whitelist, []) table = :ets.new(history_table_name(cache_name), [:set, :public, :named_table]) - :ets.insert(table, {:previous_rings, []}) - - :ok = :net_kernel.monitor_nodes(true, node_type: :all) {:ok, %{ @@ -46,7 +43,15 @@ defmodule Cache.HashRing.RingMonitor do history_size: history_size, node_blacklist: node_blacklist, node_whitelist: node_whitelist - }} + }, {:continue, :seed_and_monitor}} + end + + @impl GenServer + def handle_continue(:seed_and_monitor, %{table: table} = state) do + :ets.insert(table, {:previous_rings, []}) + :ok = :net_kernel.monitor_nodes(true, node_type: :all) + + {:noreply, state} end @impl GenServer @@ -76,7 +81,7 @@ defmodule Cache.HashRing.RingMonitor do defp safe_ets_lookup(table, key) do :ets.lookup(table, key) rescue - _ -> [] + ArgumentError -> [] end @spec history_table_name(atom()) :: atom() diff --git a/lib/cache/multi_layer.ex b/lib/cache/multi_layer.ex index b15b184..55c48b4 100644 --- a/lib/cache/multi_layer.ex +++ b/lib/cache/multi_layer.ex @@ -303,7 +303,7 @@ defmodule Cache.MultiLayer do end defp cache_module?(module) do - function_exported?(module, :get, 1) and function_exported?(module, :put, 2) + Code.ensure_loaded?(module) and function_exported?(module, :get, 1) and function_exported?(module, :put, 2) end defp invoke_callback({module, function, args}, extra_args) do diff --git a/lib/cache/persistent_term.ex b/lib/cache/persistent_term.ex index 66b60a5..f8a3d59 100644 --- a/lib/cache/persistent_term.ex +++ b/lib/cache/persistent_term.ex @@ -56,7 +56,7 @@ defmodule Cache.PersistentTerm do def get(cache_name, key, _opts \\ []) do {:ok, :persistent_term.get({cache_name, key}, nil)} rescue - exception -> + exception in ArgumentError -> {:error, ErrorMessage.internal_server_error(Exception.message(exception), %{cache: cache_name, key: key})} end @@ -66,7 +66,7 @@ defmodule Cache.PersistentTerm do :persistent_term.put({cache_name, key}, value) :ok rescue - exception -> + exception in ArgumentError -> {:error, ErrorMessage.internal_server_error(Exception.message(exception), %{cache: cache_name, key: key})} end @@ -76,7 +76,7 @@ defmodule Cache.PersistentTerm do :persistent_term.erase({cache_name, key}) :ok rescue - exception -> + exception in ArgumentError -> {:error, ErrorMessage.internal_server_error(Exception.message(exception), %{cache: cache_name, key: key})} end end diff --git a/lib/cache/refresh_ahead.ex b/lib/cache/refresh_ahead.ex index 3cb0880..80aa6fb 100644 --- a/lib/cache/refresh_ahead.ex +++ b/lib/cache/refresh_ahead.ex @@ -101,7 +101,8 @@ defmodule Cache.RefreshAhead do %{ id: :"#{cache_name}_refresh_tracker", start: {__MODULE__, :start_tracker, [tracker_name]} - } + }, + {Task.Supervisor, name: task_supervisor_name(cache_name)} ], [strategy: :one_for_one] ]} @@ -197,7 +198,7 @@ defmodule Cache.RefreshAhead do tracker = tracker_name(cache_name) if safe_ets_insert_new(tracker, {key, true}) do - Task.start(fn -> + Task.Supervisor.start_child(task_supervisor_name(cache_name), fn -> lock_resource = {:refresh_ahead_lock, cache_name, key} lock_id = {lock_resource, self()} lock_nodes = lock_nodes(adapter_opts[:lock_node_whitelist]) @@ -261,19 +262,19 @@ defmodule Cache.RefreshAhead do defp safe_ets_insert_new(tracker, record) do :ets.insert_new(tracker, record) rescue - _ -> false + ArgumentError -> false end defp safe_ets_delete(tracker, key) do :ets.delete(tracker, key) rescue - _ -> :ok + ArgumentError -> :ok end defp safe_global_set_lock(lock_id, lock_nodes) do :global.set_lock(lock_id, lock_nodes, 0) rescue - _ -> false + ArgumentError -> false end defp lock_nodes(lock_node_whitelist) do @@ -301,4 +302,6 @@ defmodule Cache.RefreshAhead do end defp tracker_name(cache_name), do: :"#{cache_name}_refresh_tracker" + + defp task_supervisor_name(cache_name), do: :"#{cache_name}_refresh_tasks" end diff --git a/lib/cache/sandbox.ex b/lib/cache/sandbox.ex index c72404e..dcbdb80 100644 --- a/lib/cache/sandbox.ex +++ b/lib/cache/sandbox.ex @@ -461,7 +461,7 @@ defmodule Cache.Sandbox do Agent.get(Cache.Sandbox, fn state -> state |> scoped_get(sid) |> Map.keys() end) rescue - _ -> [] + ArgumentError -> [] end def delete_table(cache_name) do @@ -838,6 +838,7 @@ defmodule Cache.Sandbox do end) end + # ETS counts a match only when the spec body returns exactly `true`, not any truthy term. def select_count(cache_name, match_spec) do compiled = :ets.match_spec_compile(match_spec) @@ -845,7 +846,7 @@ defmodule Cache.Sandbox do sub |> Map.to_list() |> :ets.match_spec_run(compiled) - |> Enum.count(&(&1 === true)) + |> Enum.count(&match?(true, &1)) end) end @@ -857,7 +858,7 @@ defmodule Cache.Sandbox do sub |> Map.to_list() |> Enum.split_with(fn obj -> - [obj] |> :ets.match_spec_run(compiled) |> Enum.any?(&(&1 === true)) + [obj] |> :ets.match_spec_run(compiled) |> Enum.any?(&match?(true, &1)) end) {length(to_delete), Map.new(to_keep)} @@ -896,7 +897,7 @@ defmodule Cache.Sandbox do if updates !== [] do scoped_agent_update(cache_name, fn sub -> - Enum.reduce(updates, sub, fn {k, v}, acc -> Map.put(acc, k, v) end) + Enum.reduce(updates, sub, fn {key, value}, acc -> Map.put(acc, key, value) end) end) end @@ -932,14 +933,14 @@ defmodule Cache.Sandbox do Keyword.get(info, item) end - def slot(cache_name, i) do + def slot(cache_name, index) do scoped_agent_get(cache_name, fn sub -> list = Map.to_list(sub) - if i >= length(list) do + if index >= length(list) do :"$end_of_table" else - [Enum.at(list, i)] + [Enum.at(list, index)] end end) end @@ -1162,8 +1163,7 @@ defmodule Cache.Sandbox do end def select_reverse(cache_name, match_spec) do - result = select(cache_name, match_spec) - Enum.reverse(result) + cache_name |> select(match_spec) |> Enum.reverse() end def select_reverse(cache_name, match_spec, limit) do @@ -1279,6 +1279,6 @@ defmodule Cache.Sandbox do end) end - defp enum_length(m) when is_map(m), do: m |> Map.to_list() |> enum_length() - defp enum_length(l), do: length(l) + defp enum_length(map) when is_map(map), do: map |> Map.to_list() |> enum_length() + defp enum_length(list), do: length(list) end diff --git a/lib/cache/strategy.ex b/lib/cache/strategy.ex index bb40c02..53f3d4c 100644 --- a/lib/cache/strategy.ex +++ b/lib/cache/strategy.ex @@ -82,16 +82,16 @@ defmodule Cache.Strategy do module |> module_behaviours() |> Enum.member?(Cache.Strategy) - rescue - _ -> false end + # module_info/1 raises UndefinedFunctionError for an unknown module and ArgumentError for + # a non-atom. defp module_behaviours(module) do module |> :erlang.apply(:module_info, [:attributes]) |> Keyword.get_values(:behaviour) |> List.flatten() rescue - _ -> [] + _ in [UndefinedFunctionError, ArgumentError] -> [] end end diff --git a/lib/cache/term_encoder.ex b/lib/cache/term_encoder.ex index 8c083e5..3995bb6 100644 --- a/lib/cache/term_encoder.ex +++ b/lib/cache/term_encoder.ex @@ -81,7 +81,7 @@ defmodule Cache.TermEncoder do def native_term_storage?({_strategy_module, _strategy_config}, _adapter_opts), do: true def native_term_storage?(adapter, adapter_opts) when is_atom(adapter) do - if function_exported?(adapter, :native_term_storage?, 1) do + if Code.ensure_loaded?(adapter) and function_exported?(adapter, :native_term_storage?, 1) do adapter.native_term_storage?(adapter_opts) else compile_time_native_term_storage?(adapter, adapter_opts) diff --git a/mix.exs b/mix.exs index 92b9282..792ee25 100644 --- a/mix.exs +++ b/mix.exs @@ -59,8 +59,9 @@ defmodule ElixirCache.MixProject do {:libring, "~> 1.7"}, {:credo, "~> 1.6", only: [:test, :dev], runtime: false}, {:blitz_credo_checks, "~> 0.1", only: [:test, :dev], runtime: false}, + {:mika_credo_rules, github: "MikaAK/mika_credo_rules", only: [:test, :dev], runtime: false}, {:excoveralls, "~> 0.10", only: :test}, - {:ex_doc, ">= 0.0.0", optional: true, only: :dev}, + {:ex_doc, ">= 0.0.0", optional: true, only: :dev, runtime: false}, {:dialyxir, "~> 1.0", optional: true, only: :test, runtime: false} ] end @@ -70,7 +71,7 @@ defmodule ElixirCache.MixProject do maintainers: ["Mika Kalathil"], licenses: ["MIT"], links: %{"GitHub" => "https://github.com/mikaak/elixir_cache"}, - files: ~w(mix.exs README.md CHANGELOG.md LICENSE lib) + files: ["mix.exs", "README.md", "CHANGELOG.md", "LICENSE", "lib"] ] end diff --git a/mix.lock b/mix.lock index aaccc2b..d3ee240 100644 --- a/mix.lock +++ b/mix.lock @@ -22,6 +22,7 @@ "makeup_elixir": {:hex, :makeup_elixir, "1.0.1", "e928a4f984e795e41e3abd27bfc09f51db16ab8ba1aebdba2b3a575437efafc2", [:mix], [{:makeup, "~> 1.0", [hex: :makeup, repo: "hexpm", optional: false]}, {:nimble_parsec, "~> 1.2.3 or ~> 1.3", [hex: :nimble_parsec, repo: "hexpm", optional: false]}], "hexpm", "7284900d412a3e5cfd97fdaed4f5ed389b8f2b4cb49efc0eb3bd10e2febf9507"}, "makeup_erlang": {:hex, :makeup_erlang, "1.1.0", "835f7e60792e08824cda445639555d7bf1bbbddb1b60b306e33cb6f6db24dc74", [:mix], [{:makeup, "~> 1.0", [hex: :makeup, repo: "hexpm", optional: false]}], "hexpm", "1cd6780fb1dd1a03979abaed0fe82712b0625118fd5257d3ebbf73f960c73c3c"}, "metrics": {:hex, :metrics, "1.0.1", "25f094dea2cda98213cecc3aeff09e940299d950904393b2a29d191c346a8486", [:rebar3], [], "hexpm", "69b09adddc4f74a40716ae54d140f93beb0fb8978d8636eaded0c31b6f099f16"}, + "mika_credo_rules": {:git, "https://github.com/MikaAK/mika_credo_rules.git", "1e4409f63374ba61e6c5003d42b20012e0565af2", []}, "mime": {:hex, :mime, "2.0.7", "b8d739037be7cd402aee1ba0306edfdef982687ee7e9859bee6198c1e7e2f128", [:mix], [], "hexpm", "6171188e399ee16023ffc5b76ce445eb6d9672e2e241d2df6050f3c771e80ccd"}, "mimerl": {:hex, :mimerl, "1.2.0", "67e2d3f571088d5cfd3e550c383094b47159f3eee8ffa08e64106cdf5e981be3", [:rebar3], [], "hexpm", "f278585650aa581986264638ebf698f8bb19df297f66ad91b18910dfc6e19323"}, "nimble_options": {:hex, :nimble_options, "1.1.1", "e3a492d54d85fc3fd7c5baf411d9d2852922f66e69476317787a7b2bb000a61b", [:mix], [], "hexpm", "821b2470ca9442c4b6984882fe9bb0389371b8ddec4d45a9504f00a66f650b44"}, diff --git a/test/cache/case_template_test.exs b/test/cache/case_template_test.exs index 628f2b1..b05b7c7 100644 --- a/test/cache/case_template_test.exs +++ b/test/cache/case_template_test.exs @@ -17,7 +17,7 @@ defmodule Cache.CaseTemplateTest do opts: [] end - defmodule TestCacheCase do + defmodule TestCacheCaseTemplate do use Cache.CaseTemplate, default_caches: [TestCache] end @@ -82,9 +82,12 @@ defmodule Cache.CaseTemplateTest do end +# IsolationTest and ExtraCachesTest share the TestCache sandbox Agent, and +# Cache.SandboxRegistry.start/1 adopts an already-running Agent into the calling test's +# supervisor, so one test's teardown would kill the other's cache if they ran concurrently. defmodule Cache.CaseTemplateTest.IsolationTest do - use ExUnit.Case - use Cache.CaseTemplateTest.TestCacheCase + use ExUnit.Case, async: false + use Cache.CaseTemplateTest.TestCacheCaseTemplate alias Cache.CaseTemplateTest.TestCache @@ -100,7 +103,7 @@ defmodule Cache.CaseTemplateTest.IsolationTest do end defmodule Cache.CaseTemplateTest.ExtraCachesTest do - use ExUnit.Case + use ExUnit.Case, async: false use Cache.CaseTemplateTest.TestCaseCaseWithExtra, caches: [Cache.CaseTemplateTest.TestCache2] alias Cache.CaseTemplateTest.TestCache diff --git a/test/cache/con_cache_test.exs b/test/cache/con_cache_test.exs index d06804a..032ab51 100644 --- a/test/cache/con_cache_test.exs +++ b/test/cache/con_cache_test.exs @@ -55,6 +55,18 @@ defmodule Cache.ConCacheTest do %{key: key} end + describe "adapter calls after the ConCache table is gone" do + test "put and delete return an error tuple instead of raising" do + cache_name = :"orphan_con_cache_#{System.unique_integer([:positive])}" + start_supervised!({ConCache, name: cache_name, ttl_check_interval: false}) + :ets.delete(ConCache.ets(cache_name)) + + assert {:error, %ErrorMessage{code: :internal_server_error}} = Cache.ConCache.put(cache_name, :key, "value") + assert {:error, %ErrorMessage{code: :internal_server_error}} = Cache.ConCache.put(cache_name, :key, 1_000, "value") + assert {:error, %ErrorMessage{code: :internal_server_error}} = Cache.ConCache.delete(cache_name, :key) + end + end + describe "&get_or_store/3" do test "get/set", %{key: key} do assert "test_value" === ConCacheAdapter.get_or_store(key, @ttl, fn -> "test_value" end) @@ -64,12 +76,9 @@ defmodule Cache.ConCacheTest do test "uses fetch function exactly once", %{key: key} do assert "VALUE" === ConCacheAdapter.get_or_store(key, @ttl, fn -> - Process.sleep(1_000) "VALUE" end) - Process.sleep(200) - assert "VALUE" === ConCacheAdapter.get_or_store(key, @ttl, fn -> raise "not used" end) end end diff --git a/test/cache/counter_test.exs b/test/cache/counter_test.exs index 5d95fb8..c938541 100644 --- a/test/cache/counter_test.exs +++ b/test/cache/counter_test.exs @@ -10,7 +10,6 @@ defmodule Cache.CounterTest do setup do start_supervised({Cache, [TestCounterCache]}) - Process.sleep(50) :ok end @@ -164,6 +163,16 @@ defmodule Cache.CounterTest do end end + describe "adapter calls against a counter that was never started" do + test "every operation returns an error tuple instead of raising" do + assert {:error, %ErrorMessage{code: :internal_server_error}} = Cache.Counter.get(:no_such_counter, 0) + assert {:error, %ErrorMessage{code: :internal_server_error}} = Cache.Counter.put(:no_such_counter, 0, 1) + assert {:error, %ErrorMessage{code: :internal_server_error}} = Cache.Counter.delete(:no_such_counter, 0) + assert {:error, %ErrorMessage{code: :internal_server_error}} = Cache.Counter.increment(:no_such_counter, 0) + assert {:error, %ErrorMessage{code: :internal_server_error}} = Cache.Counter.decrement(:no_such_counter, 0) + end + end + describe "get with non-integer key" do test "returns error for atom key" do assert {:error, %ErrorMessage{code: :bad_request}} = TestCounterCache.get(:some_atom) diff --git a/test/cache/dets_test.exs b/test/cache/dets_test.exs index 657ff1b..ee05e66 100644 --- a/test/cache/dets_test.exs +++ b/test/cache/dets_test.exs @@ -10,8 +10,6 @@ defmodule Cache.DETSTest do setup do start_supervised({Cache, [TestDETSCache]}) - Process.sleep(100) - on_exit(fn -> File.rm("/tmp/test_dets_cache.dets") end) @@ -19,6 +17,18 @@ defmodule Cache.DETSTest do :ok end + describe "start_link/1 with a file that is not a dets table" do + test "returns the open_file reason instead of crashing the caller" do + Process.flag(:trap_exit, true) + file_path = "/tmp/corrupt_dets_#{System.unique_integer([:positive])}.dets" + File.write!(file_path, "not a dets file") + on_exit(fn -> File.rm(file_path) end) + + assert {:error, {:dets_open_file, {:not_a_dets_file, _path}}} = + Cache.DETS.start_link(table_name: :corrupt_dets_cache, file_path: file_path) + end + end + describe "match_object/2" do test "matches objects in the table" do TestDETSCache.insert_raw({:key1, "SomeRandomValue"}) diff --git a/test/cache/ets_test.exs b/test/cache/ets_test.exs index 9ce68a2..14b4b72 100644 --- a/test/cache/ets_test.exs +++ b/test/cache/ets_test.exs @@ -12,8 +12,6 @@ defmodule Cache.ETSTest do setup do start_supervised({Cache, [TestETSCache]}) - Process.sleep(100) - :ok end @@ -163,11 +161,42 @@ defmodule Cache.ETSTest do } ) - Process.sleep(100) - assert {:ok, "persisted_value"} === RehydrateTestCache.get(:persisted_key) end + test "creates a new table when the file holds a different table name" do + File.mkdir_p!("/tmp/ets_test") + on_exit(fn -> File.rm_rf("/tmp/ets_test") end) + + :ets.new(:some_other_table, [:set, :public, :named_table]) + :ets.insert(:some_other_table, {:persisted_key, "other"}) + :ets.tab2file(:some_other_table, ~c"/tmp/ets_test/rehydrate_test_cache.ets") + :ets.delete(:some_other_table) + + start_supervised!(%{ + id: :rehydrate_cache_sup, + type: :supervisor, + start: {Cache, :start_link, [[RehydrateTestCache], [name: :rehydrate_cache_sup]]} + }) + + assert {:ok, nil} === RehydrateTestCache.get(:persisted_key) + end + + test "creates a new table when the file is not an ets dump" do + File.mkdir_p!("/tmp/ets_test") + on_exit(fn -> File.rm_rf("/tmp/ets_test") end) + File.write!("/tmp/ets_test/rehydrate_test_cache.ets", "not an ets dump") + + start_supervised!(%{ + id: :rehydrate_cache_sup, + type: :supervisor, + start: {Cache, :start_link, [[RehydrateTestCache], [name: :rehydrate_cache_sup]]} + }) + + assert :ok === RehydrateTestCache.put(:fresh_key, "fresh") + assert {:ok, "fresh"} === RehydrateTestCache.get(:fresh_key) + end + test "creates new table when no file exists" do File.mkdir_p!("/tmp/ets_test") on_exit(fn -> File.rm_rf("/tmp/ets_test") end) @@ -180,14 +209,20 @@ defmodule Cache.ETSTest do } ) - Process.sleep(100) - assert {:ok, nil} === NewTableTestCache.get(:nonexistent_key) assert :ok === NewTableTestCache.put(:new_key, "new_value") assert {:ok, "new_value"} === NewTableTestCache.get(:new_key) end end + describe "adapter calls against a table that does not exist" do + test "get, put and delete return an error tuple instead of raising" do + assert {:error, %ErrorMessage{code: :internal_server_error}} = Cache.ETS.get(:no_such_ets_table, :key) + assert {:error, %ErrorMessage{code: :internal_server_error}} = Cache.ETS.put(:no_such_ets_table, :key, "value") + assert {:error, %ErrorMessage{code: :internal_server_error}} = Cache.ETS.delete(:no_such_ets_table, :key) + end + end + describe "all/0" do test "returns a list of ETS tables including the test table" do tables = TestETSCache.all() @@ -228,7 +263,6 @@ defmodule Cache.ETSTest do start: {Cache, :start_link, [[DeleteTableCache], [name: :delete_table_sup]]} }) - Process.sleep(100) DeleteTableCache.insert_raw({:key, "val"}) assert DeleteTableCache.delete_table() === true end @@ -357,7 +391,6 @@ defmodule Cache.ETSTest do start: {Cache, :start_link, [[RenameTestCache], [name: :rename_test_sup]]} }) - Process.sleep(100) RenameTestCache.insert_raw({:rkey, "rval"}) RenameTestCache.rename(:renamed_cache) assert :ets.lookup(:renamed_cache, :rkey) === [{:rkey, "rval"}] @@ -493,7 +526,6 @@ defmodule Cache.ETSTest do start: {Cache, :start_link, [[File2TabTestCache], [name: :file2tab_sup]]} }) - Process.sleep(100) File2TabTestCache.insert_raw({:fb_key, "val"}) file = ~c"/tmp/test_ets_file2tab_#{:rand.uniform(100_000)}" diff --git a/test/cache/hash_ring_monitor_test.exs b/test/cache/hash_ring_monitor_test.exs index 6b746da..62f70db 100644 --- a/test/cache/hash_ring_monitor_test.exs +++ b/test/cache/hash_ring_monitor_test.exs @@ -68,7 +68,7 @@ defmodule Cache.HashRing.RingMonitorTest do }) send(pid, :some_random_message) - Process.sleep(50) + _synced = :sys.get_state(pid) assert Process.alive?(pid) end @@ -83,11 +83,11 @@ defmodule Cache.HashRing.RingMonitorTest do }) send(pid, {:nodeup, :"fake@node", %{}}) - Process.sleep(50) + _synced = :sys.get_state(pid) assert Process.alive?(pid) send(pid, {:nodedown, :"fake@node", %{}}) - Process.sleep(50) + _synced = :sys.get_state(pid) assert Process.alive?(pid) end end diff --git a/test/cache/hash_ring_test.exs b/test/cache/hash_ring_test.exs index b687d2a..84469d5 100644 --- a/test/cache/hash_ring_test.exs +++ b/test/cache/hash_ring_test.exs @@ -15,8 +15,6 @@ defmodule Cache.HashRingTest do start: {Cache, :start_link, [[TestHashRingCache], [name: :hash_ring_cache_sup]]} }) - Process.sleep(50) - :ok end @@ -150,8 +148,6 @@ defmodule Cache.HashRingTest do Cache.HashRing.get(cache_name, "repair:dedup_key", Cache.ETS, rpc_module: rpc_module) - Process.sleep(50) - assert Agent.get(call_count, & &1) === 1 end @@ -200,10 +196,11 @@ defmodule Cache.HashRingTest do Cache.HashRing.get(cache_name, "repair:migrate_key2", Cache.ETS, rpc_module: rpc_module) - Process.sleep(100) - - calls = Agent.get(calls_agent, & &1) - assert Enum.any?(calls, fn {_node, func} -> func === :delete end) + assert Cache.Wait.until(fn -> + calls_agent + |> Agent.get(& &1) + |> Enum.any?(fn {_node, func} -> func === :delete end) + end) end end @@ -283,7 +280,7 @@ defmodule Cache.HashRingTest do [self() | callers ++ ancestors] |> Enum.filter(&is_pid/1) |> Enum.find(fn pid -> - :persistent_term.get({Cache.HashRingTest.StubRpc, pid}, nil) !== nil + not is_nil(:persistent_term.get({Cache.HashRingTest.StubRpc, pid}, nil)) end) end end diff --git a/test/cache/multi_layer_broadcast_test.exs b/test/cache/multi_layer_broadcast_test.exs index 6b1116a..ecdca77 100644 --- a/test/cache/multi_layer_broadcast_test.exs +++ b/test/cache/multi_layer_broadcast_test.exs @@ -1,5 +1,5 @@ defmodule Cache.MultiLayerBroadcastTest do - use ExUnit.Case + use ExUnit.Case, async: true alias Cache.MultiLayer.Coordinator diff --git a/test/cache/refresh_ahead_test.exs b/test/cache/refresh_ahead_test.exs index 123b23d..60d1a1b 100644 --- a/test/cache/refresh_ahead_test.exs +++ b/test/cache/refresh_ahead_test.exs @@ -34,6 +34,9 @@ defmodule Cache.RefreshAheadTest do def refresh(key), do: {:ok, "locked:#{key}"} end + # Every fixture cache uses refresh_before: 500, so this TTL is inside the window at age 0. + @in_window_ttl 500 + setup do start_supervised!(%{ id: :refresh_cache_sup, @@ -86,58 +89,80 @@ defmodule Cache.RefreshAheadTest do assert :ok === TestRefreshCache.put("no_refresh_key", 10_000, "original") assert {:ok, "original"} === TestRefreshCache.get("no_refresh_key") - - Process.sleep(50) - assert {:ok, "original"} === TestRefreshCache.get("no_refresh_key") end + # A TTL no larger than refresh_before puts the key inside the refresh window from the + # first read: that read returns the stale value and spawns the async refresh. test "triggers async refresh when within refresh_before window" do - assert :ok === TestRefreshCache.put("refresh_key", 2000, "original") - - assert {:ok, "original"} === TestRefreshCache.get("refresh_key") - - Process.sleep(1600) + assert :ok === TestRefreshCache.put("refresh_key", @in_window_ttl, "original") assert {:ok, "original"} === TestRefreshCache.get("refresh_key") - Process.sleep(200) - - assert {:ok, "refreshed:refresh_key"} === TestRefreshCache.get("refresh_key") + assert Cache.Wait.until(fn -> + TestRefreshCache.get("refresh_key") === {:ok, "refreshed:refresh_key"} + end) end test "on_refresh option overrides module callback" do - assert :ok === CallbackRefreshCache.put("cb_key", 2000, "original") + assert :ok === CallbackRefreshCache.put("cb_key", @in_window_ttl, "original") assert {:ok, "original"} === CallbackRefreshCache.get("cb_key") - Process.sleep(1600) + assert Cache.Wait.until(fn -> + CallbackRefreshCache.get("cb_key") === {:ok, "custom:cb_key"} + end) + end + end - assert {:ok, "original"} === CallbackRefreshCache.get("cb_key") + describe "deduplication" do + # The refresh callback counts its calls and then blocks until the test opens the gate, + # so every concurrent get runs while one refresh is provably still in flight. + defmodule DedupRefreshCache do + use Cache, + adapter: {Cache.RefreshAhead, Cache.ETS}, + name: :dedup_refresh_cache, + opts: [refresh_before: 500] + + def refresh(key) do + Agent.update(__MODULE__.Calls, &(&1 + 1)) + Cache.Wait.until(fn -> Agent.get(__MODULE__.Gate, & &1) end, 5_000) + {:ok, "refreshed:#{key}"} + end + end + + setup do + start_supervised!(%{ + id: :dedup_refresh_cache_sup, + type: :supervisor, + start: {Cache, :start_link, [[DedupRefreshCache], [name: :dedup_refresh_cache_sup]]} + }) - Process.sleep(200) + start_supervised!(%{id: :calls, start: {Agent, :start_link, [fn -> 0 end, [name: DedupRefreshCache.Calls]]}}) + start_supervised!(%{id: :gate, start: {Agent, :start_link, [fn -> false end, [name: DedupRefreshCache.Gate]]}}) - assert {:ok, "custom:cb_key"} === CallbackRefreshCache.get("cb_key") + :ok end - end - describe "deduplication" do test "multiple concurrent gets only spawn one refresh task" do - assert :ok === TestRefreshCache.put("dedup_key", 2000, "original") + assert :ok === DedupRefreshCache.put("dedup_key", @in_window_ttl, "original") - Process.sleep(1600) + results = + 1..5 + |> Enum.map(fn _ -> Task.async(fn -> DedupRefreshCache.get("dedup_key") end) end) + |> Task.await_many() - tasks = - Enum.map(1..5, fn _ -> - Task.async(fn -> TestRefreshCache.get("dedup_key") end) - end) + assert Enum.all?(results, &(&1 === {:ok, "original"})) - results = Task.await_many(tasks) - assert Enum.all?(results, fn {:ok, val} -> val === "original" end) + # The one refresh task is parked at the gate, so the count cannot climb past 1. + assert Cache.Wait.until(fn -> Agent.get(DedupRefreshCache.Calls, & &1) >= 1 end) + assert Agent.get(DedupRefreshCache.Calls, & &1) === 1 - Process.sleep(200) + Agent.update(DedupRefreshCache.Gate, fn _ -> true end) - assert {:ok, "refreshed:dedup_key"} === TestRefreshCache.get("dedup_key") + assert Cache.Wait.until(fn -> + DedupRefreshCache.get("dedup_key") === {:ok, "refreshed:dedup_key"} + end) end test "global lock prevents refresh while lock is held" do @@ -146,13 +171,10 @@ defmodule Cache.RefreshAheadTest do lock_nodes = [Node.self()] assert true === :global.set_lock(lock_id, lock_nodes, 0) - assert :ok === LockedRefreshCache.put("locked_key", 2000, "original") - - Process.sleep(1600) + assert :ok === LockedRefreshCache.put("locked_key", @in_window_ttl, "original") assert {:ok, "original"} === LockedRefreshCache.get("locked_key") - - Process.sleep(250) + assert Cache.Wait.until(fn -> refresh_finished?(:locked_refresh_cache, "locked_key") end) assert {:ok, "original"} === LockedRefreshCache.get("locked_key") @@ -160,15 +182,15 @@ defmodule Cache.RefreshAheadTest do # clean itself up before releasing — otherwise it can reach :global.set_lock after # the del_lock below, take the freed lock, and refresh the value we assert is still # untouched. - Process.sleep(250) + assert Cache.Wait.until(fn -> refresh_finished?(:locked_refresh_cache, "locked_key") end) assert true === :global.del_lock(lock_id, lock_nodes) assert {:ok, "original"} === LockedRefreshCache.get("locked_key") - Process.sleep(250) - - assert {:ok, "locked:locked_key"} === LockedRefreshCache.get("locked_key") + assert Cache.Wait.until(fn -> + LockedRefreshCache.get("locked_key") === {:ok, "locked:locked_key"} + end) end end @@ -198,16 +220,10 @@ defmodule Cache.RefreshAheadTest do end test "uses MFA tuple for refresh callback" do - assert :ok === MFARefreshCache.put("mfa_key", 2000, "original") - assert {:ok, "original"} === MFARefreshCache.get("mfa_key") - - Process.sleep(1600) - + assert :ok === MFARefreshCache.put("mfa_key", @in_window_ttl, "original") assert {:ok, "original"} === MFARefreshCache.get("mfa_key") - Process.sleep(200) - - assert {:ok, "mfa:mfa_key"} === MFARefreshCache.get("mfa_key") + assert Cache.Wait.until(fn -> MFARefreshCache.get("mfa_key") === {:ok, "mfa:mfa_key"} end) end end @@ -242,15 +258,35 @@ defmodule Cache.RefreshAheadTest do end test "works with atom whitelist" do - assert :ok === AtomWhitelistCache.put("awl_key", 2000, "original") - - Process.sleep(1600) + assert :ok === AtomWhitelistCache.put("awl_key", @in_window_ttl, "original") assert {:ok, "original"} === AtomWhitelistCache.get("awl_key") - Process.sleep(200) + assert Cache.Wait.until(fn -> + AtomWhitelistCache.get("awl_key") === {:ok, "atom_wl:awl_key"} + end) + end + end + + # The refresh task clears its tracker entry in an `after` block, so an absent entry means + # the spawned task has finished — whether it refreshed or lost the lock race. + defp refresh_finished?(cache_name, key) do + not :ets.member(:"#{cache_name}_refresh_tracker", key) + end + + describe "strategy calls on a cache whose tracker table does not exist" do + test "get/4 still returns the stored value and skips the refresh" do + :ets.new(:orphan_refresh_cache, [:set, :public, :named_table]) + wrapped = {"original", System.monotonic_time(:millisecond), @in_window_ttl} + :ets.insert(:orphan_refresh_cache, {"key", wrapped}) + + assert {:ok, "original"} === + Cache.RefreshAhead.get(:orphan_refresh_cache, "key", Cache.ETS, refresh_before: 500) + end - assert {:ok, "atom_wl:awl_key"} === AtomWhitelistCache.get("awl_key") + test "delete/4 returns the underlying adapter's error instead of raising" do + assert {:error, %ErrorMessage{code: :internal_server_error}} = + Cache.RefreshAhead.delete(:no_such_refresh_cache, "key", Cache.ETS, []) end end diff --git a/test/cache_strategy_test.exs b/test/cache_strategy_test.exs index 23258ed..fe1c292 100644 --- a/test/cache_strategy_test.exs +++ b/test/cache_strategy_test.exs @@ -74,8 +74,6 @@ defmodule CacheStrategyTest do ]} }) - Process.sleep(50) - :ok end @@ -87,8 +85,6 @@ defmodule CacheStrategyTest do assert {:ok, nil} = cache_module.get(test_key) assert :ok = cache_module.put(test_key, value) - Process.sleep(50) - assert {:ok, value} === cache_module.get(test_key) end @@ -99,12 +95,8 @@ defmodule CacheStrategyTest do assert :ok = cache_module.put(test_key, value) - Process.sleep(50) - assert :ok = cache_module.delete(test_key) - Process.sleep(50) - assert {:ok, nil} = cache_module.get(test_key) end @@ -116,13 +108,9 @@ defmodule CacheStrategyTest do assert {:ok, nil} = cache_module.get(test_key) assert :ok = cache_module.put(test_key, value) - Process.sleep(50) - assert {:ok, value} === cache_module.get(test_key) assert :ok = cache_module.put(test_key, nil) - Process.sleep(50) - assert {:ok, nil} = cache_module.get(test_key) end end @@ -140,8 +128,6 @@ defmodule CacheStrategyTest do ]} }) - Process.sleep(50) - :ok end @@ -152,8 +138,6 @@ defmodule CacheStrategyTest do assert :ok = cache_module.put(test_key, value) - Process.sleep(50) - assert {:ok, value} === cache_module.get_or_create(test_key, fn -> raise "I shouldn't be called" @@ -174,8 +158,6 @@ defmodule CacheStrategyTest do {:ok, value} end) - Process.sleep(50) - assert {:ok, value} === cache_module.get(test_key) end end @@ -236,8 +218,6 @@ defmodule CacheStrategyTest do ]} }) - Process.sleep(50) - :ok end @@ -284,8 +264,6 @@ defmodule CacheStrategyTest do ]} }) - Process.sleep(50) - :ok end @@ -319,8 +297,6 @@ defmodule CacheStrategyTest do [[TestCache.MultiLayerFetch], [name: :multi_layer_fetch_sup]]} }) - Process.sleep(50) - :ok end diff --git a/test/cache_test.exs b/test/cache_test.exs index 43e0342..96ebcb9 100644 --- a/test/cache_test.exs +++ b/test/cache_test.exs @@ -34,7 +34,8 @@ defmodule CacheTest do setup do start_supervised({Cache, [unquote(adapter)]}) - Process.sleep(100) + # Cache.Redis connects asynchronously, so wait for a read to succeed before testing. + assert Cache.Wait.until(fn -> match?({:ok, _}, unquote(adapter).get(:readiness_probe)) end) :ok end @@ -47,8 +48,6 @@ defmodule CacheTest do assert {:ok, nil} = cache_module.get(test_key) assert :ok = cache_module.put(test_key, value) - Process.sleep(50) - assert {:ok, value} === cache_module.get(test_key) end @@ -59,12 +58,8 @@ defmodule CacheTest do assert :ok = cache_module.put(test_key, value) - Process.sleep(50) - assert :ok = cache_module.delete(test_key) - Process.sleep(50) - assert {:ok, nil} = cache_module.get(test_key) end @@ -76,13 +71,9 @@ defmodule CacheTest do assert {:ok, nil} = cache_module.get(test_key) assert :ok = cache_module.put(test_key, value) - Process.sleep(50) - assert {:ok, value} === cache_module.get(test_key) assert :ok = cache_module.put(test_key, nil) - Process.sleep(50) - assert {:ok, nil} = cache_module.get(test_key) end end @@ -91,7 +82,8 @@ defmodule CacheTest do setup do start_supervised({Cache, [unquote(adapter)]}) - Process.sleep(100) + # Cache.Redis connects asynchronously, so wait for a read to succeed before testing. + assert Cache.Wait.until(fn -> match?({:ok, _}, unquote(adapter).get(:readiness_probe)) end) :ok end @@ -103,8 +95,6 @@ defmodule CacheTest do assert :ok = cache_module.put(test_key, value) - Process.sleep(50) - assert {:ok, value} === cache_module.get_or_create(test_key, fn -> raise "I shouldn't be called" @@ -125,8 +115,6 @@ defmodule CacheTest do {:ok, value} end) - Process.sleep(50) - assert {:ok, value} === cache_module.get(test_key) end end @@ -162,8 +150,6 @@ defmodule CacheTest do test "returns application env config with name" do options = [host: "localhost", port: 6379] - Application.put_env(:elixir_cache, TestCache.RedisRuntimeAppEnv, options) - defmodule TestCache.RedisRuntimeAppEnv do use Cache, adapter: Cache.Redis, @@ -177,8 +163,6 @@ defmodule CacheTest do test "returns application env config with name and key" do options = [host: "localhost", port: 6379] - Application.put_env(:elixir_cache, :cache, options) - defmodule TestCache.RedisRuntimeAppEnvKey do use Cache, adapter: Cache.Redis, diff --git a/test/support/wait.ex b/test/support/wait.ex new file mode 100644 index 0000000..8b1ee3c --- /dev/null +++ b/test/support/wait.ex @@ -0,0 +1,23 @@ +defmodule Cache.Wait do + @moduledoc """ + Polls a condition so tests synchronize on observable state instead of sleeping. + """ + + @poll_interval_ms 10 + + @doc "Returns `true` as soon as `fun` returns a truthy value, or `false` once `timeout` ms elapse." + @spec until((-> as_boolean(term())), non_neg_integer()) :: boolean() + def until(fun, timeout \\ 1_000) do + poll(fun, System.monotonic_time(:millisecond) + timeout) + end + + defp poll(fun, deadline) do + cond do + fun.() -> true + System.monotonic_time(:millisecond) >= deadline -> false + true -> + Process.sleep(@poll_interval_ms) + poll(fun, deadline) + end + end +end diff --git a/test/test_helper.exs b/test/test_helper.exs index 42d0522..575a2c1 100644 --- a/test/test_helper.exs +++ b/test/test_helper.exs @@ -1,3 +1,7 @@ +# App-env fixtures for the `opts: :app` / `opts: {:app, :key}` cases in test/cache_test.exs +Application.put_env(:elixir_cache, CacheTest.TestCache.RedisRuntimeAppEnv, host: "localhost", port: 6379) +Application.put_env(:elixir_cache, :cache, host: "localhost", port: 6379) + Cache.SandboxRegistry.start_link() ExUnit.start()