Add Elixir Registry support for Finch instance naming#366
Add Elixir Registry support for Finch instance naming#366meox wants to merge 1 commit intosneako:mainfrom
Conversation
NelsonVides
left a comment
There was a problem hiding this comment.
Question, this is intended to help with the case when you create dynamic Finch instances at runtime, but, wouldn't a single instance with dynamic pools do the work?
We've recently introduced this feature, it's already in main and to be released to hex soon enough. This PR introduces quite some complexity so I'd like to understand what does this solve that the new recent features don't 🤔
|
the main goal is to start the pool at runtime without generating atoms |
|
Yeah, but you can start pools dynamically at runtime without generating atoms already, see #345 and all the linked PRs, doesn't that exactly solve the issue already or is there anything missing? 🤔 |
I missed that part so probably we have to adapt the Req lib to take advantage of that? |
|
It hasn't made it to a hex package yet, I think we're just missing out a Mint release here #341, merge that, and Finch should be ready. Jose and Wojtek are aware of these changes (Jose proposed them!) so Req is probably adapting to this too already, but I haven't followed Req's changes :) Thanks a lot for your PR nevertheless! |
Add Elixir Registry support for Finch instance naming
Problem
Finch only accepts
atom()for the:nameoption instart_link/1. This forcesusers who dynamically create Finch pools from runtime configuration (e.g., config
files, databases, multi-tenant setups) to use
String.to_atom/1, which leaks atomssince the BEAM never garbage-collects them. This is a well-known footgun for any
system with unbounded dynamic names.
Solution
Allow
{:via, Registry, {registry, key}}tuples as the:nameoption, followingthe standard Elixir convention for dynamic process naming without atom creation.
Design
Finch's internals (Elixir
Registry.lookup/2, named ETS tables) require atomidentifiers — this is a hard constraint from the BEAM and Elixir's
Registrymodule (
@type registry :: atom). The solution introduces a thin indirectionlayer:
Finch.NameRegistry— a shared ETS table (:finch_via_names) that maps{:via, Registry, {reg, key}}tuples to auto-generated internal atoms(e.g.,
:"Finch.Instance.42"). These atoms are bounded by the number ofactive Finch instances, not by user input.
Finch.ViaCleaner— a tiny GenServer added as a child of the Finchsupervision tree (only when using via tuples) that removes the ETS mapping
entry on termination, preventing stale entries.
resolve_finch_name/1— called at the entry point of every public APIfunction. For atoms it's a no-op passthrough; for via tuples it resolves to
the internal atom via the ETS lookup.
What changes
lib/finch/name_registry.exlib/finch/via_cleaner.exlib/finch.ex@type name(),start_link/1,init/1,finch_name!/1; addresolve_finch_name/1; wire resolution into all public API functionslib/finch/pool/manager.exget_pool_supervisor/2: catch:noprocexit when supervisor dies between Registry lookup andSupervisor.count_children/1test/finch_test.exs"Registry-based naming"describe block with teststest/finch/http1/pool_test.exsassert_receivetimeouts from 100-150ms to 500msWhat stays unchanged
Pool.Manager,Pool.Supervisor,HTTP1.Pool,HTTP2.Pool,PoolMetrics) — they only ever receive atoms after resolutionresolve_finch_name/1foratoms is a single pattern match)
Tests
get_pool_status/2with via tuple namestart_pool/3with via tuple namestop_pool/2with via tuple nameget_pool_count/2andset_pool_count/3with via tuple nameFlaky test fixes
get_pool_supervisor/2race condition —Supervisor.count_children/1wascalled on a PID obtained from
Registry.lookup/2without guarding against thesupervisor dying between lookup and call. Fixed by catching
:noprocexits andreturning
:not_found.HTTP1 pool idle timeout tests —
assert_receivetimeouts of 100-150ms weretoo tight under parallel test load. Increased to 500ms. The
refute_receivetimeouts (which test that something does NOT happen within a window) are left
unchanged since they need to be tight to be meaningful.