Support :on_timeout in Task.async_stream and friends by whatyouhide · Pull Request #6009 · elixir-lang/elixir

whatyouhide · 2017-04-17T17:04:48Z

:on_timeout can be :kill_task | :exit, where :exit is the current behaviour.

josevalim · 2017-04-18T10:04:30Z

Maybe ":exit (default) - ..."?

This way we don't need the trailing "Defaults to :exit.".

Good point, will update.

josevalim · 2017-04-18T10:06:25Z

Don't we log some messages when we kill?

Mh, I don't think so. Should we?

No, I was just asking/checking. :)

fishcakez

Given the 2 possible ways to leak timers I think we need to send the timer to the Monitor, instead of the Caller.

I believe that sending the timer/cancelling in the Monitor would also allow us to run slightly faster in a multi core scenario because we can run slightly more efficiently:

cancel timer using async: true, info: false so we don't block to cancel (this might be a call to timer wheel on another scheduler) as we wouldn't care for the result and don't need to flush in Monitor
send pid to Caller then start timer (don't block to add to timer wheel)

fishcakez · 2017-04-18T22:19:28Z

When starting a timer for a different process it is not possible to guarantee that the other process receives the timer reference because it is always possible for the process to get an exit signal in between starting the timer and sending the timer reference. Therefore to prevent timer leaks we would require the different process to always go down, i.e. using links, monitors or supervisors. Unfortunately can't guarantee this here.

It could happen in the following situation:

on_timeout: :exit and catches exit when stream enumerated

Monitor process gets exit signal that makes it exit between Process.send_after and send

Caller is trapping exits when it receives the exit signal from monitor process (need not be when spawning monitor process but still possible if :kill)

fishcakez · 2017-04-18T22:30:44Z

If the caller kills the timed out task and is not trapping exits it will get :killed exit signal from the propagating links. Would it be more useful to have semantics that match Task.yield and Task.shutdown, where we would have nil on timeout? (Note that Task.shutdown does a safe unlink+shutdown/kill).

nil + safe kill sounds okay to me, @josevalim thoughts?

Yes and yes.

We should also test the safe kill, possibly by not trapping exits during the tests.

fishcakez · 2017-04-18T22:35:57Z

There can be pending timers in our waiting map, we would need to cancel all these timers on stream_close, as the Caller might catch the exit/1.

fishcakez · 2017-04-25T10:03:49Z

          case waiting do
            %{^position => {_, {:ok, _} = ok}} -> Map.put(waiting, position, {nil, ok})
            %{^position => {_, :running}} -> Map.put(waiting, position, {nil, {:exit, reason}})
+            %{^position => {_, :timed_out}} -> Map.put(waiting, position, {nil, {:exit, :killed}})


We can not guarantee the reason is :killed here, it can be any reason. For example task can exit while timer message is in the message queue of the Monitor. Since it would be :killed if it was killed we could just use the reason in the :exit. Also the reason might be :killed because the task was killed for another cause.

Therefore I think we should use another value when :timed_out and reason is :killed, otherwise do {:exit, reason}.

@fishcakez okay, got it. So one option would be to Process.exit(task, :kill) but emit {:exit, :killed_for_timeout}, and another option would be to Process.exit(task, :kill) and emit {:exit, reason_task_died}, did I get it correctly? If so, which one do you think is better?

fishcakez · 2017-04-25T10:10:34Z

+        case running_tasks do
+          %{^ref => {position, _type, pid, _timer_ref}} ->
+            send(parent_pid, {:killed_for_timeout, {monitor_ref, position}})
+            Process.exit(pid, :kill)


If we aren't trapping exits and on_timeout: :exit this creates 2 possibilities.

Either the task is killed and the exit signals propagates causing Caller to get :killed exit signal. Or the Caller handles the :killed_for_timeout message and tells the Monitor to stop all tasks. The Monitor receives the stop and then traps exits before task exits. Then the Caller will call exit({:timeout, ..}). This differing semantics is complex. I think we would need to unlink from the Monitor, perhaps similar to Task.shutdown.

fishcakez · 2017-04-25T21:34:54Z

            send(parent_pid, {:killed_for_timeout, {monitor_ref, position}})
+            caller = self()
+            ref = make_ref()
+            enforcer = spawn(fn ->


If trapping exits we could skip spawning, unsure if worth complexity of that optimisation though.

fishcakez · 2017-04-26T12:10:15Z

            %{^position => {_, {:ok, _} = ok}} -> Map.put(waiting, position, {nil, ok})
            %{^position => {_, :running}} -> Map.put(waiting, position, {nil, {:exit, reason}})
-            %{^position => {_, :timed_out}} -> Map.put(waiting, position, {nil, {:exit, :killed}})
+            %{^position => {_, :timed_out}} -> Map.put(waiting, position, {nil, {:exit, :timed_out}})


GenServer.start_link does {:error, :timeout} : https://github.com/erlang/otp/blob/cd412d911efbda23e7dd3aef5cf910defc886211/lib/stdlib/src/proc_lib.erl#L350

Should we use :timeout as well?

Sure, works for me. @josevalim okay for you?

Need new final review.

josevalim · 2017-04-27T09:00:47Z

          case waiting do
            %{^position => {_, {:ok, _} = ok}} -> Map.put(waiting, position, {nil, ok})
            %{^position => {_, :running}} -> Map.put(waiting, position, {nil, {:exit, reason}})
+            %{^position => {_, :timed_out}} -> Map.put(waiting, position, {nil, {:exit, :timeout}})


Maybe we should use :timeout here too for consistency?

I thought about that, I wasn't sure. I don't have any argument for :timed_out here though, so I updated to use :timeout. Should I change the message we send with Process.send_after/3 as well?

Probably yes :), I updated that as well. Let me know how it looks.

josevalim

It looks good to me. If @fishcakez agree, we shall merge it.

whatyouhide · 2017-04-28T19:15:50Z

🎉!

whatyouhide requested review from fishcakez and josevalim April 17, 2017 17:04

josevalim reviewed Apr 18, 2017

View reviewed changes

josevalim previously approved these changes Apr 18, 2017

View reviewed changes

fishcakez previously requested changes Apr 18, 2017

View reviewed changes

whatyouhide added 5 commits April 23, 2017 09:09

:on_timeout

c34fc36

Flush :timed_out messages

16e84d7

Add tests

9f3c7f6

Add docs

7ab878a

WIP

d1e8075

fishcakez reviewed Apr 25, 2017

View reviewed changes

Enforce killing the task

550f52d

fishcakez reviewed Apr 25, 2017

View reviewed changes

{:exit, :timed_out}

e107633

fishcakez reviewed Apr 26, 2017

View reviewed changes

whatyouhide added 2 commits April 26, 2017 23:10

:timeout

33cdb58

Update docs

6dbad72

whatyouhide requested review from fishcakez and josevalim April 26, 2017 21:12

josevalim reviewed Apr 27, 2017

View reviewed changes

:timed_out -> :timeout

e01fce5

josevalim approved these changes Apr 27, 2017

View reviewed changes

fishcakez approved these changes Apr 28, 2017

View reviewed changes

whatyouhide merged commit dfd0ab5 into elixir-lang:master Apr 28, 2017

whatyouhide deleted the on-timeout branch April 28, 2017 19:15

Conversation

whatyouhide commented Apr 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fishcakez left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fishcakez Apr 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

whatyouhide Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

whatyouhide Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josevalim left a comment

Choose a reason for hiding this comment

Uh oh!

whatyouhide commented Apr 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

fishcakez left a comment •

edited

Loading

fishcakez Apr 18, 2017 •

edited

Loading

whatyouhide Apr 19, 2017 •

edited

Loading

whatyouhide Apr 27, 2017 •

edited

Loading