Proposal: Add `cancel` opt to assign_async and start_async

[aglassman]

Problem

The cancel_async function is easily overlooked. Since the results of “outdated” tasks are ignored, it’s easy for developers to assume that the task was killed, even though it will continue to execute. The async_assign documentation does not make a reference to cancel_async, although start_async does.

I think this default is setting up users for issues down the road. Many times, these async functions are used to perform slow, or expensive database queries. Given that each task will checkout its own connection, this puts the LiveView at risk for putting undue pressure on the connection pool.

Suggestions

Suggestion 1

Add mention of cancel_async to the async_assign documentation.

Suggestion 2

assign_async and start_async should provide an opt called cancel, which would effectively do what cancel_async does.

usage: assign_async(socket, :records, fn → … end, cancel: true)

Suggestion 3

Provide a configuration to set the default for the cancel opt. This would allow a sensible default for the workload of the project, yet could still be overridden. This also provides an easy way to not break existing behavior for LiveView projects. The library default can be cancel_async_default: false, and teams can then determine if modifying this value makes sense. This minimizes the amount of code changes needed for an existing project, and removes the need for individuals on a team to remember to add cancel: ... to every usage of async_assign or start_async.

config :phoenix_live_view,
   cancel_async_default: true

[rhcarvalho]

@aglassman

Suggestion 1

Add mention of cancel_async to the async_assign documentation.

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

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

@aglassman

Suggestion 2

assign_async and start_async should provide an opt called cancel, which would effectively do what cancel_async does.

usage: assign_async(socket, :records, fn → … end, cancel: true)

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.

[garrison]

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.

[tfwright]

@aglassman

Suggestion 3

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.

[derek-zhou]

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.

[garrison]

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

[aglassman]

@rhcarvalho

Suggestion: cancel_existing: true or replace: true.

Great suggestion, agree here.

@rhcarvalho

if you haven’t already, you can implement this in your project and report on the usage.

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

@garrison

you’re unlikely to glean any additional performance killing the task a few milliseconds earlier than it would have completed

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).

@garrison

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.

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.

@derek-zhou

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.

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.

[rhcarvalho]

@aglassman

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

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!

Feature Request: add Telemetry events for `assign_async`

Hey, @andypearson I share a hackish starting point for you to play with: Create a module with your new code, which can reference the Phoenix.LiveView implementation and augment it: defmodule MyAppWeb.Helpers do require Phoenix.LiveView def assign_async(socket, key_or_keys, func, opts \\ []) do dbg(binding()) Phoenix.LiveView.assign_async(socket, key_or_keys, func, opts) end end Update your “MyAppWeb” module with the override: defmodule MyAppWeb do #... existing code ... …

[tfwright]

@garrison

Wrapper functions are another option.

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.

[derek-zhou]

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.

[garrison]

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.

[krasenyp]

Remember using Redux Saga way back when. The way it works doesn’t matter but you could fire multiple actions in a short amount of time and use the result of the latest action. I think it was called take_latest. It’s a fitting name.

[garrison]

The async assign functionality in LiveView is very well-designed and handles this case out of the box; the new assigns/tasks always clobber the old ones.

This proposal is purely a performance optimization. I don’t think it matters most of the time because most async tasks are probably very short and do not benefit from cancellation. However, the OP has hard data proving they are the exception and I cannot argue with that.