Better Async Effect Cleanup

Current Situation

Presently, the "cleanup" behavior for async effects is that they are cancelled before the next effect takes place or the component is unmounted. While this behavior may be desirable in many cases, it does not give the user the ability to gracefully deal with cleanup logic in the way one can with a sync event.

For example, consider a long running async effect:

@use_effect
async def long_running_task():
    while True:
        await something()
        await something_else()

The problem with this current behavior is that handling the cancellation is messy. You could do the following:

@use_effect
async def long_running_task():
    try:
        while True:
            await something()
            await something_else()
    finally:
        ...  # cleanup logic here

However, this may lead to some confusing behavior since it's not possible to know whether something() or something_else() will receive the cancellation. You might have to do a lot of extra work to figure out what state your program was left in after the cancellation.

Proposed Actions

Thankfully the above only impacts async effects. With a synchronous event, you would be able to handle async task cleanup using something like the following:

@use_effect
def start_long_running_task():
    interupt = Event()
    asyncio.create_task(long_running_task(interupt))
    return cleanup.set

async def long_running_task(interrupt):
    while not interupt.is_set():
        await something()
        await something_else()
    ...  # cleanup logic here

Given this, our proposed solution to the problem is to allow async effects to accept a similar interupt asyncio.Event:

@use_effect
async def long_running_task(interrupt):
    while not interupt.is_set():
        await something()
        await something_else()
    ...  # cleanup logic here

@Archmonger

@Archmonger I'm realizing that replicating the prior cancellation behavior is quite tricky to do with the interupt event - it would almost always be better to just write a sync event in that case. Perhaps we should preserve the original behavior where we cancel the effect if it does not not accept any arguments?

That was my original justification for the async_schedule parameter (name TBD). It simplifies awkward situations like that.

I think it's less ambiguous than just assuming cancellation on argless functions. With this, it might cover enough to where we don't need the interrupt parameter.

cancel-oldest # The current behavior
queue-all # Best for something that needs to run in the background every time, no matter what
no-requeue # Best for long polling functions

The interrupt event is definitely necessary since that's what allows the user to implement cleanup logic after the event has been set. I'm also realizing that effects must run one after the other - that is, we must wait for the the prior effect to complete before kicking off the next one. Otherwise you could end up in a weird situation where the cleanup logic for the last effect might be running while the setup logic for the next one does.

Otherwise you could end up in a weird situation where the cleanup logic for the last effect might be running while the setup logic for the next one does.

I believe race conditions are expected with asyncio, and should be a problem for the user to solve.

I'm not a huge fan of an implict cancellation policy on argless functions...

Maybe we have a cancellable parameter in the use_effect call?

@use_effect(cancellable=True)
async def do_something(interrupt):
   ...

But then that kinda makes interrupt useless in this context anyways... Maybe we really do have to just support argless async effects. Either that or support async_schedule, which isn't ideal either.

I believe race conditions are expected with asyncio, and should be a problem for the user to solve.

The problem is that, with the proposed API, the user doesn't actually have enough control to manage this themselves. We would need to pass in a reference to the prior task so they could await it if they wanted:

@use_effect
def my_effect(prior: Task | None, interupt: Event):
    if prior is not None:
        await prior
    # do stuff
    await interupt.wait()
    # clean up

This may actually be the interface that provides the necessary level of control. With a reference to the prior task you could cancel it if you wanted.

If we think we might need even more options in the future, we can consider a dataclass called something like EffectHandler containing both of these args.

Should this be closed in favor of Deprecate async effects issue?

Probably

Cancellation shielding may be the answer to this interface question. In short, the current behavior would be maintained. That is, async effects are cancelled before the next one is executed. Users can then leverage cancellation shielding to protect important tasks from cancellation if necessary.

With that said, we should, but currently don't, wait for the completion of old cancelled effects.

Fundamentally, work being done by an effect as a result of an old render should be stopped. The main issue for users is doing so gracefully since dealing with the unpredictable timing of cancellations is challenging. Thus, the remedy may simply be good documentation that explains this.

Navigation Menu

Repository navigation

Better Async Effect Cleanup #956

Current Situation

Proposed Actions

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Better Async Effect Cleanup #956

Description

Current Situation

Proposed Actions

Activity

rmorshea commented on Mar 16, 2023

Archmonger commented on Mar 16, 2023

rmorshea commented on Mar 16, 2023

Archmonger commented on Mar 17, 2023

rmorshea commented on Mar 17, 2023

Archmonger commented on Mar 19, 2023

Archmonger commented on Apr 3, 2023

rmorshea commented on Apr 3, 2023

rmorshea commented on Apr 9, 2023

rmorshea commented on Apr 9, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions