Correctness/safety question with DynamicSupervisor

I think you should look at the registry module. It will allow you to give arbitrary identifiers to the processes you spawn dynamically.

Registry is implemented with ets which should help with concerns about race conditions.

You want to decouple the supervision tree concerns from the key management and uniqueness. For the former continue to use DynamicSupervisor. For the latter you can use Registry if you only need to enforce uniqueness on a per node level.

Global will synchronously ping every other node to register the name which is probably not something you want in a user path. I don’t really get the impression that it’s meant for a large number of processes, but someone should correct me if I’m wrong.

Consider using Elixir’s Registry as mentioned above or writing your own routing layer. You probably also want to maintain limits and rate limit users creating rooms.

Right now, I’m definitely only doing single-node servers (hence my method just raising whenever it’s called against a remote supervisor); but I am interested in building this in a way that will not cause a giant headache if the code is ever upgraded to support multi-node servers.

The users definitely do not care about any backend node partitioning; the room IDs should be a genuinely global identifier. I absolutely must never put the users in a position where 2 different users who attempted to join the same room end up attached to different “MyApp.Room” instances. So does that mean Registry would not work for this?

The Elixir Registry is local, yes. I don’t think it would be very hard to migrate off of, though.

You can write your own routing table/registry fairly easily with a GenServer and ets as primitives. Then you can use :global to ensure your routing table is registered on one node and send all requests to that node.

That will get you pretty far. If you need to scale further you can have multiple processes read from the ets table, and if you need to scale out much further you can shard your rooms across nodes and route locally on each node with a node id embedded in the room id or similar. Probably you don’t even need to think about doing this right now.

As already mentioned the Registry modules works per node. But if you build your system using via tuples it should be not much work to switch to a different registry later. Generally you want to use a registry – a module allowing you to register a process/pid for a given key – no matter which specific registry you land on in the end. OTP comes with :global, elixir has Registry and there are third party ones each with different sets of tradeoffs.

We have just rewritten the Mix & OTP guides in Elixir to build something similar, you can read it in this PR. We also built something that relied on similar techniques here: Homemade analytics with Ecto and Elixir - Dashbit Blog

I don’t know how long it will be until that PR makes it into hexdocs.pm, but for now it can be read at https://github.com/elixir-lang/elixir/blob/ce64fc364d368705759d0982b72a4ba98572caa6/lib/elixir/pages/mix-and-otp/agents.md#preview.

Does this seem to be a correct implementation of those ideas to implement a DynamicSupervisor with unique children, or is this still going to fail in edge cases?

defmodule MyApp.Util.DynamicSupervisor2 do
  # https://forum.elixirforum.com/t/correctness-safety-question-with-dynamicsupervisor/71652/9
  use Agent

  @type supervisor :: Agent.agent
  @type on_terminate :: :ok | {:error, :not_found}

  def count_children(supervisor) do
    Agent.get(supervisor, fn %{supervisor: supervisor} ->
      DynamicSupervisor.count_children(supervisor)
    end)
  end

  @spec get_or_start_child(
    supervisor(), Supervisor.module_spec() | Supervisor.child_spec()
  ) :: DynamicSupervisor.on_start_child()
  def get_or_start_child(supervisor, child_spec)

  def get_or_start_child(supervisor, child_spec = %{id: id, start: {_, _, _}}) do
    Agent.get(supervisor, fn %{
      supervisor: supervisor,
      registry: registry,
      options: opts
    } ->
      case Registry.lookup(registry, id) do
        [{_, pid}] ->
          {:ok, pid}

        [] ->
          case DynamicSupervisor.start_child(supervisor, child_spec) do
            {:ok, pid} ->
              {:ok, _} = Registry.register(registry, id, pid)
              {:ok, pid}

            :ignore ->
              :ignore

            {:error, {:already_started, pid}} = result ->
              if Keyword.get(opts, :allow_foreign_child, false) do
                {:ok, _} = Registry.register(registry, id, pid)
                {:ok, pid}
              else
                result
              end

            {:error, _} = result ->
              result
          end
      end
    end)
  end

  def get_or_start_child(supervisor, module) when is_atom(module) do
    # https://github.com/elixir-lang/elixir/blob/v1.19.0-rc.0/lib/elixir/lib/gen_server.ex#L1036
    get_or_start_child(supervisor, module.child_spec([]))
  end

  def get_or_start_child(supervisor, {module, arg}) when is_atom(module) do
    get_or_start_child(supervisor, module.child_spec(arg))
  end

  def start_link(opts) do
    allow_foreign = Keyword.get(opts, :allow_foreign, false)

    {name, opts} = Keyword.pop(opts, :name, __MODULE__)
    registry_name = Module.concat(name, "registry")

    agent_opts = [name: name]
    supervisor_opts = [strategy: :one_for_one]
    registry_opts = [keys: :unique, name: registry_name]

    Agent.start_link(
      fn ->
        supervisor = case DynamicSupervisor.start_link(supervisor_opts) do
          {:ok, pid} ->
            pid

          {:error, {:already_started, pid}} when allow_foreign ->
            pid
        end

        case Registry.start_link(registry_opts) do
          {:ok, pid} ->
            pid

          {:error, {:already_started, pid}} when allow_foreign ->
            pid
        end

        %{
          supervisor: supervisor,
          registry: registry_name,
          options: opts
        }
      end,
      agent_opts
    )
  end

  @spec terminate_child(supervisor(), atom()) :: on_terminate()
  def terminate_child(supervisor, id) when is_atom(id) do
    Agent.get(supervisor, fn %{
      supervisor: supervisor,
      registry: registry
    } ->
      case Registry.lookup(registry, id) do
        [{_, pid}] ->
          :ok = DynamicSupervisor.terminate_child(supervisor, pid)
          :ok = Registry.unregister(registry, id)
          :ok

        [] ->
          {:error, :not_found}
      end
    end)
  end
end

The updated docs are here: Simple state with agents — Elixir v1.21.0-dev

We generally treat naming processes and supervising processes as separate things. You don’t need to wrap the registry and supervisor access from within the agent. The architecture built in the getting started guide should be enough and provide uniqueness (either locally or in a cluster).