Raise when reaching max chidren in Task.Supervisor.async_stream #8239

whatyouhide · 2018-09-30T18:27:23Z

Closes #7786.

whatyouhide · 2018-09-30T18:29:24Z

lib/elixir/lib/task/supervised.ex

@@ -411,6 +411,19 @@ defmodule Task.Supervised do
        send(pid, {self(), {monitor_ref, spawned}})
        Map.put(waiting, spawned, {pid, :running})

+      {:max_children, ^monitor_ref} ->
+        stream_cleanup_inbox(monitor_pid, monitor_ref)
+        Process.demonitor(monitor_ref, [:flush])


This is the thing I'm the least sure about. Basically, if the monitor process can't spawn a task because :max_children, it will kill all other running tasks and send {:max_children, monitor_ref} to the parent, then it will exit(:normal). The only other cases it exit(:normal)s is when the parent process tells it to stop, but here we can't use stream_close (and tell the monitor process to stop) because we need to stop all processes and the monitor process right away. Demonitoring here works but I'm not sure it's what we want since we don't do it anywhere else. Definitely open to better solutions.

I think there is no guarantee the parent process is dead by the time we call stream_cleanup_inbox. Therefore, if someone rescues this exception, we will have leaked messages to the inbox, since we are trapping exits. I would prefer if we could trigger stream_close from here and go through the usual shutdown procedure.

In a nutshell, the monitor will deliver max_children and start a new loop. Then we invoke stream_close here, which will deliver the stop message to terminate. And then we raise the error below. Thoughts?

Sounds like a better solution yes.

@josevalim pushed a commit that fixes this. I think I did what you suggested. There's a bit of code repetition but it should work. Thoughts?

fertapric · 2018-10-01T07:11:20Z

lib/elixir/lib/task/supervised.ex

+        reached the maximum number of tasks for this task supervisor. The maximum number \
+        of tasks that are allowed to run at the same time under this supervisor can be \
+        configured with the :max_children option passed to Task.Supervisor.start_link/1. When \
+        using async_stream, make sure to configure :max_concurrency to be lower or equal to \


Minor nitpick: should async_stream_nolink be mentioned here?

josevalim · 2018-10-05T08:48:03Z

lib/elixir/lib/task/supervised.ex

+
+          {:error, :max_children} ->
+            send(parent_pid, {:max_children, monitor_ref})
+            stream_waiting_for_stop_loop(running_tasks, config)


You can remove the duplication by adding a catch all to this receive, something like:

... other -> handle_stop_or_exit(other, running_tasks, config) stream_monitor_loop(running_tasks, config)

And then you just need to create a stream_stop_loop that reuses the same logic.

Okay. I am not a huge fan of this solution because 1. it complicates the loop by adding indirection since now you don't see all possible messages and 2. it implies that handle_stop_or_exit exits the monitor process. But it works. I pushed a commit, if you like it we can get that in in the meantime and I can try and think of a cleaner solution.

fertapric · 2018-10-05T22:58:58Z

lib/elixir/lib/task/supervised.ex

    end
  end

+  defp stream_waiting_for_stop_loop(running_tasks, config) do
+    %{
+      parent_ref: parent_ref,


I think these are no longer used

fertapric · 2018-10-05T23:06:26Z

lib/elixir/lib/task/supervised.ex

+    } = config
+
+    receive do
+      message ->


Just to double check: this was matching before bbef098 on:

{:stop, ^monitor_ref}

{:DOWN, ^parent_ref, _, _, reason}

_other (match all)

However, I think it now only matches on:

{:stop, ^monitor_ref} (the first clause of handle_stop_or_parent_down)

{:DOWN, ^parent_ref, _, _, reason} (the second clause of handle_stop_or_parent_down)

The previous match-all is gone. Could that produce undesired FunctionClauseErrors?

It will lead to undefined FunctionClauseError. We need to ignore the message in case it is not the one we are looking for.

The problem with this is that before the stream_monitor_loop didn't accept random messages as the catch-all clause was {:EXIT, ...}. Not sure how to approach this.

You can make handle_stop_or_parent_down return true (if handled) or false (if unknown) and raise accordingly. handle_stop... || raise "unexpected message #{inspect other}".

Yeah that makes sense. I'm now raising in stream_monitor_loop but not in stream_waiting_for_stop_loop. Should work :)

josevalim · 2018-10-06T07:12:07Z

lib/elixir/lib/task/supervised.ex

+
+  defp handle_stop_or_parent_down({:DOWN, parent_ref, _, _, reason}, running_tasks, %{
+         parent_ref: parent_ref
+       }) do


We rarely use this pattern in Elixir, can we please keep all arguments in a single line or one per line? Thanks. Same for the clauses above and then tests.

josevalim · 2018-10-06T16:21:21Z

lib/elixir/lib/task/supervised.ex

+
+      other ->
+        handle_stop_or_parent_down(other, running_tasks, config) ||
+          raise "unexpected message: #{inspect(other)}"


Actually, this may not be a good idea. Could observer or other tools for example try to send it system message? 🤔 /cc @fishcakez

Well we weren't handling those before either, so might be something for another PR?

Before they would sit in the process inbox, now they will cause a crash.

I think it is best if we revert the last commit and simply discard messages.

Argh, that's correct. Reverted.

This reverts commit c31d0aa.

Raise when reaching max chidren in Task.Supervisor.async_stream

e8321ff

Closes #7786.

whatyouhide commented Sep 30, 2018

View reviewed changes

fertapric reviewed Oct 1, 2018

View reviewed changes

FIXUP

4b7a1e9

josevalim reviewed Oct 5, 2018

View reviewed changes

FIXUP

bbef098

fertapric reviewed Oct 5, 2018

View reviewed changes

josevalim reviewed Oct 6, 2018

View reviewed changes

whatyouhide added 2 commits October 6, 2018 11:43

FIXUP

fc49c33

FIXUP

c31d0aa

josevalim reviewed Oct 6, 2018

View reviewed changes

whatyouhide added 3 commits October 6, 2018 20:04

Revert "FIXUP"

d64319b

This reverts commit c31d0aa.

FIXUP formatting

dec1ddb

FIXUP formatting 2.0

46d49e6

josevalim approved these changes Oct 6, 2018

View reviewed changes

whatyouhide merged commit ef72328 into master Oct 6, 2018

whatyouhide deleted the al/raise-max-children-async-stream branch October 6, 2018 18:48

fertapric mentioned this pull request Oct 9, 2018

Send and handle kills concurrently in Task.Supervised #8260

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raise when reaching max chidren in Task.Supervisor.async_stream #8239

Raise when reaching max chidren in Task.Supervisor.async_stream #8239

whatyouhide commented Sep 30, 2018

whatyouhide Sep 30, 2018

josevalim Oct 1, 2018

whatyouhide Oct 4, 2018

whatyouhide Oct 4, 2018

fertapric Oct 1, 2018 •

edited

josevalim Oct 5, 2018

whatyouhide Oct 5, 2018

fertapric Oct 5, 2018

fertapric Oct 5, 2018

josevalim Oct 6, 2018

whatyouhide Oct 6, 2018

josevalim Oct 6, 2018

whatyouhide Oct 6, 2018

josevalim Oct 6, 2018

josevalim Oct 6, 2018

whatyouhide Oct 6, 2018

josevalim Oct 6, 2018

josevalim Oct 6, 2018

whatyouhide Oct 6, 2018

Raise when reaching max chidren in Task.Supervisor.async_stream #8239

Raise when reaching max chidren in Task.Supervisor.async_stream #8239

Conversation

whatyouhide commented Sep 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fertapric Oct 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fertapric Oct 1, 2018 •

edited