And perhaps even add to the overall Async Operations too: Phoenix.LiveView — Phoenix LiveView v1.2.5.

I imagine none of the maintainers would oppose to a PR doing just that!

I see a case where you’d want to possibly kill existing tasks started earlier, similar to what is described in the start_async/4 docs. So your intention is to make killing in-flight tasks the default, right?

I think naming the option :cancel is confusing, as reading the code in isolation makes me think of cancelling of the new task I’m about to start.
Suggestion: cancel_existing: true or replace: true.

Idea: if you haven’t already, you can implement this in your project and report on the usage. MyApp.Async.assign_async/4 can replicate the assign_async/4 signature, but additionally understand a replace: true option which makes it call cancel_async/3 before delegating to assign_async/4.

Now that I look at it, I see one thing this option would miss is the ability to pass a custom reason when cancelling the task.

I like that LiveView gives us the primitives. I’m not so sure that replacing in-flight tasks should be the default. If it was, and there was no cancel_async/3, a reasonable ask would be how to keep in-flight tasks.

For most simple tasks canceling by default is more effort than it’s worth. That is, for a simple async assign like “fetch a row from the DB” you’re unlikely to glean any additional performance killing the task a few milliseconds earlier than it would have completed. What’s important is that the newer task’s result clobbers the old one, and LiveView already does the right thing there.

For long-running tasks canceling makes more sense, but these are less common so I think the current default is quite reasonable.

Ask for the cancel option (cancel_existing would indeed be clearer), I’m not really sold either way.

socket
|> cancel_async(:foo)
|> start_async(:foo, fn _ ->
  # ...
end)

socket
|> start_async(:foo, fn _ ->
  # ...
end, cancel_existing: true)

The only meaningful advantage here is that you don’t have to write the key twice. But I could see that being a slight improvement in some cases.

I like this suggestion. Don’t know enough about the internal of the async assigns implementation to consider downsides, and I suspect there might be some if it is not already the current behavior. As a user it certainly seems cleaner for the system to take care of cancelling any ongoing processes related to the previous assign for me. I’ve done something similar most of the time when creating APIs that manage async processes–if the user makes a second request while the first is ongoing, cancel it before starting the new one.

Function calls with a inline closure and a keyword list are ugly. I suggest to name the new behaviors, sans the option, reassign_async and restart_async and keep the old behaviors to the old functions.

Lol actually I think the inline closure plus kwlist looks quite good, so I guess this is a case of aesthetic preference.

Wrapper functions are another option. You could even just do it yourself:

def cancel_start_async(socket, key, fun) do
  socket
  |> cancel_async(key)
  |> start_async(key, fun)
end

Great suggestion, agree here.

Yes, I think I will try it out and report back.

I agree here. The benefit of killing the task as the default is mainly when DB usage is spiked / an unexpected issue is impacting query performance. The users starts clicking around the page, spawning many tasks that are just putting pressure on the connection pool. Or, a LiveView is subscribed to a PubSub topic, and the code broadcasting has a bug that starts emitting events at an elevated rate. (Both are real situations I’ve encountered, and using cancel_async fixed most of the issues).

Agreed, this is why I thought a global default (suggestion 3) would be an advantage. Make the default the “safest” option, while still giving the individual the option to override explicitly.

I wouldn’t be opposed to that option. Refactoring existing code to use this wouldn’t be too difficult, and it would be clear to developers that it manages tasks differently than async_assign / start_async.

One way to accomplish this for your whole app/team, with a similar effect than the config you suggested, is to replace assign_async and start_async in your MyAppWeb module.

I wrote more details with a code example in the forum before, turns out it was also about assign_async!

Based on how core teams in the ecosystem have responded to similar proposals, I think this is the most likely response. And although I grumble it makes sense. When Elixir makes it so easy to make such things ergonomic while keeping them explicit and minimizing API surface, probably best to prefer that.

The problem with inline closure and a keyword list as arguments are they both prefer to be the last of the argument list. In the case of keyword list, you can skip the [] and it sorta looks like named arguments. In the case of inline closures, they could be long and anything beyond the non-trivial closure will feel out-of-place.

I guess I see what you mean. Honestly I don’t think it looks too bad. Maybe if the closure was really long, but at that point I’d probably split it up.

In this case the discussion is kinda moot, though, as the function already takes a kwlist.