elixir-lang / gen_stage

Producer and consumer actors with back-pressure for Elixir
http://hexdocs.pm/gen_stage
1.52k stars 193 forks source link

DynamicSupervisor ignores min_demand #71

Closed hunterboerner closed 8 years ago

hunterboerner commented 8 years ago

The min_demand option—while stored and computed—is ignored in DynamicSupervisor. See: https://github.com/elixir-lang/gen_stage/blob/master/lib/dynamic_supervisor.ex#L510-L516

Demo:

defmodule DynSupDemo do
  alias Experimental.DynamicSupervisor
  use DynamicSupervisor

  def start_link do
    DynamicSupervisor.start_link(__MODULE__, [], name: __MODULE__)
  end

  def init([]) do
    children = [
      worker(Consumer, [], restart: :temporary)
    ]

    {:ok, children, [strategy: :one_for_one,
                     subscribe_to: [{Producer,
                                     min_demand: 999,
                                     max_demand: 1000
                                    }]]}
  end
end

defmodule Producer do
  use Experimental.GenStage
  alias Experimental.{GenStage, DynamicSupervisor}

  def start_link do
    GenStage.start_link(__MODULE__, 1, name: __MODULE__)
  end

  def init(counter) do
    {:producer, counter}
  end

  def handle_demand(demand, counter) when demand > 0 do
    IO.puts "==>#{demand} --- #{DynamicSupervisor.count_children(DynSupDemo).active}"
    # This exists to stagger the event completion (see Consumer)
    counter = cond do
      counter >= 10 -> 1
      counter -> counter
    end
    list = Enum.to_list(counter..(demand - 1 + counter))
    IO.inspect list
    {:noreply, list, counter + demand}
  end
end

defmodule Consumer do
  use GenServer

  def start_link(event) do
    GenServer.start_link(__MODULE__, event)
  end

  def init(args) do
    send(self(), :process)
    {:ok, args}
  end

  def handle_info(:process, state) do
    IO.inspect state
    # Stagger completion.
    :timer.sleep(3_00 + (state * 1_00))
    {:stop, :normal, state}
  end
end

You can see that even though min_demand is set to 999, the DynamicSupervisor will ask for more events after each event is processed. Is this the intended behavior, a bug, or am I using DynamicSupervisor for the wrong purpose?

josevalim commented 8 years ago

I am not sure i understand the question. If the question is: does the DynamicSupervisor only asks for more events after each event is processed, then the answer is yes. It is exactly the same behaviour as a regular consumer: you only ask for more items after you consumed the current events. So max_demand allows you to set a maximum limit on how many dynamic children you would have. Did you have another use case in mind?

hunterboerner commented 8 years ago

If I understand correctly, min_demand allows you to specify how many events to process before asking for more. So if max is 1000 and min is 500 then it will ask for 1000, process 500, then ask for 500 more. What this example shows is that DynamicSupervisor is ignoring the min_demand and instead just asking for one more event at a time as each is processed. Is my understanding of min_demand incorrect?

On Sun, Aug 14, 2016, 6:51 PM José Valim notifications@github.com wrote:

I am not sure i understand the question. If the question is: does the DynamicSupervisor only asks for more events after each event is processed, then the answer is yes. It is exactly the same behaviour as a regular consumer: you only ask for more items after you consumed the current events. So max_demand allows you to set a maximum limit on how many dynamic children you would have. Did you have another use case in mind?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/elixir-lang/gen_stage/issues/71#issuecomment-239706614, or mute the thread https://github.com/notifications/unsubscribe-auth/ABQNR6fjmR2PICuiVI6PX8T3Ou1yyt94ks5qf6oZgaJpZM4JkAuc .

josevalim commented 8 years ago

If you set the min_demand to 999, then that's exactly what will happen, no?

On Monday, August 15, 2016, Theron Boerner notifications@github.com wrote:

If I understand correctly, min_demand allows you to specify how many events to process before asking for more. So if max is 1000 and min is 500 then it will ask for 1000, process 500, then ask for 500 more. What this example shows is that DynamicSupervisor is ignoring the min_demand and instead just asking for one more event at a time as each is processed. Is my understanding of min_demand incorrect?

On Sun, Aug 14, 2016, 6:51 PM José Valim <notifications@github.com javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

I am not sure i understand the question. If the question is: does the DynamicSupervisor only asks for more events after each event is processed, then the answer is yes. It is exactly the same behaviour as a regular consumer: you only ask for more items after you consumed the current events. So max_demand allows you to set a maximum limit on how many dynamic children you would have. Did you have another use case in mind?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/elixir-lang/gen_stage/issues/71# issuecomment-239706614, or mute the thread https://github.com/notifications/unsubscribe-auth/ ABQNR6fjmR2PICuiVI6PX8T3Ou1yyt94ks5qf6oZgaJpZM4JkAuc .

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/elixir-lang/gen_stage/issues/71#issuecomment-239709167, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAlbsYMLCIqzD1TvKFGQHjPFhkgHKx_ks5qf7PBgaJpZM4JkAuc .

José Valimwww.plataformatec.com.br http://www.plataformatec.com.br/Founder and Director of R&D

fishcakez commented 8 years ago

I think what @hunterboerner is tryng to say is that demand can be sent too early because demand can be sent for any min_demand and not just 999 once the first child exits.

hunterboerner commented 8 years ago

Yes, what @fishcakez said.