Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better Async Effect Cleanup #956

Open
rmorshea opened this issue Mar 16, 2023 · 11 comments · May be fixed by #1169
Open

Better Async Effect Cleanup #956

rmorshea opened this issue Mar 16, 2023 · 11 comments · May be fixed by #1169
Labels
priority-2-moderate Should be resolved on a reasonable timeline. release-patch Warrents a patch release type-feature About new capabilities type-revision About a change in functionality or behavior
Milestone

Comments

@rmorshea
Copy link
Collaborator

rmorshea commented Mar 16, 2023

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
@rmorshea rmorshea added flag-triage Not prioritized. type-feature About new capabilities priority-2-moderate Should be resolved on a reasonable timeline. type-revision About a change in functionality or behavior release-patch Warrents a patch release and removed flag-triage Not prioritized. labels Mar 16, 2023
@rmorshea
Copy link
Collaborator Author

rmorshea commented Mar 16, 2023

@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?

@rmorshea rmorshea added this to the 1.1.0 milestone Mar 16, 2023
@Archmonger
Copy link
Contributor

Archmonger commented Mar 16, 2023

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

@rmorshea
Copy link
Collaborator Author

rmorshea commented Mar 16, 2023

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.

@Archmonger
Copy link
Contributor

Archmonger commented Mar 17, 2023

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.

@rmorshea
Copy link
Collaborator Author

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.

@Archmonger
Copy link
Contributor

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.

@Archmonger Archmonger linked a pull request Apr 3, 2023 that will close this issue
3 tasks
@Archmonger
Copy link
Contributor

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

@rmorshea
Copy link
Collaborator Author

rmorshea commented Apr 3, 2023

Probably

@rmorshea
Copy link
Collaborator Author

rmorshea commented Apr 9, 2023

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.

@rmorshea
Copy link
Collaborator Author

rmorshea commented Apr 9, 2023

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.

@Archmonger
Copy link
Contributor

Archmonger commented Apr 12, 2023

I'm in favor of deprecating async effects. It's fairly easy to manage an async task within a sync effect.

Any async design we create is going to be over complicated. And even worse, it will be a syntactical mismatch between ReactJS and ReactPy.

I think we wait for when/if ReactJS ever supports async effects to re-add support.

This was referenced Jul 4, 2023
@rmorshea rmorshea linked a pull request Dec 9, 2023 that will close this issue
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-2-moderate Should be resolved on a reasonable timeline. release-patch Warrents a patch release type-feature About new capabilities type-revision About a change in functionality or behavior
Projects
None yet
2 participants