MatchError in Task.Supervisor.async/1 in case of custom max_children option

[van-mronov]

Environment

• Elixir & Erlang/OTP versions (elixir --version):
  Erlang/OTP 21 [erts-10.0] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [hipe] [dtrace]

Elixir 1.7.0-dev (c3b0caa) (compiled with Erlang/OTP 21)

• Operating system:
  macOS High Sierra

Current behavior

iex(1)> fun = fn -> receive do: (true -> true) end
#Function<20.127694169/0 in :erl_eval.expr/5>
iex(2)> {:ok, pid} = Task.Supervisor.start_link(max_children: 2)
{:ok, #PID<0.107.0>}
iex(3)> Task.Supervisor.async(pid, fun)
%Task{
  owner: #PID<0.104.0>,
  pid: #PID<0.109.0>,
  ref: #Reference<0.1607025971.2254700546.50905>
}
iex(4)> Task.Supervisor.async(pid, fun)
%Task{
  owner: #PID<0.104.0>,
  pid: #PID<0.111.0>,
  ref: #Reference<0.1607025971.2254700546.50934>
}
iex(5)> Task.Supervisor.async(pid, fun)
** (MatchError) no match of right hand side value: {:error, :max_children}
    (elixir) lib/task/supervisor.ex:376: Task.Supervisor.async/6

Expected behavior

Task.Supervisor.async/1 and other calls to start async task should return {:error, :max_children} if DynamicSupervisor cannot start child.

[josevalim]

Good catch. Not sure about the behaviour here. We can't return {:error, :max_children} because we just return the task, so that would be confusing. So we may have to raise but something more meaningful.

[van-mronov]

Yeah, I see. Also we have the same issue with async_stream calls. And, perhaps, raising error in spawn function makes sense.

[josevalim]

async_stream allows us to return errors, so it is not a big problem there.

[van-mronov]

So, what if add something like MaxChildrenError?

[josevalim]

@van-mronov we generally do not add new exceptions for cases like this. But it may be worth if it can be raised on async and used as a signal on async_stream.

[michalmuskala]

The interaction between a task supervisor with max_children and async_stream seems very fuzzy to me.

A possible desired behaviour there could be that if the child can't be started right away the stream will block until it can start (or the timeout fires). Returning :error tuple doesn't seem very useful, since it would need to return it for all the following elements most of the time - if it couldn't start one, it probably won't be able to start another one immediately after.

[van-mronov]

Perhaps, it makes sense to use max_children (if it's specified) as default value for max_concurrency option of async_stream.

[josevalim]

Perhaps, it makes sense to use max_children (if it's specified) as default value for max_concurrency option of async_stream.

If you are using max_concurrency, i would say there is no reason to set max_children. Maybe we should clarify this in the docs.

[josevalim]

Closing in favor of PR.