reactive-python / reactpy

It's React, but in Python
https://reactpy.dev
MIT License
7.89k stars 317 forks source link

better async effects #1169

Open rmorshea opened 11 months ago

rmorshea commented 11 months ago

By submitting this pull request you agree that all contributions to this project are made under the MIT license.

Issues

Closes: https://github.com/reactive-python/reactpy/issues/956

Solution

Effects can now be defines as sync or async generators (more info in the docs):

@use_effect
def my_effect():
    conn = open_connection()
    try:
        # do something with the connection
        yield
    finally:
        conn.close()

@use_effect
async def my_effect():
    conn = await open_connection()
    try:
        # do something with the connection
        yield
    finally:
        await conn.close()

Note: this contains a subset of the changes originally created in https://github.com/reactive-python/reactpy/pull/1093

Checklist

Archmonger commented 11 months ago

Will review on Monday. Not home at the moment.

rmorshea commented 11 months ago

I think we can have use_async_effect with a timeout parameter, but something I realized while working on this is that I don't think we can have a default. Having a default timeout trades concerns about responsiveness and hangs for memory leaks and unexpected behavior due to premature timeouts.

Archmonger commented 11 months ago

Django has a similar timeout design for views. They have an arbitrary default of 20 seconds. ASGI webservers also have similar timeouts, specifically related to keep-alive requests. Usually default of 5 seconds.

We just have to pick a "good enough" default, and trust the user to change it when needed.

Archmonger commented 11 months ago

On that note, maybe teardown_timeout is a more appropriate name.

rmorshea commented 11 months ago

We just have to... trust the user to change it when needed.

Ultimately we have to trust the user to know what they're doing in some regard. Either we trust them to understand that if their effects don't end in a timely manner it will impact the responsiveness of components, or we trust that they understand how premature timeouts may cause memory leaks and unexpected behavior when resources aren't cleaned up. My intuition is that debugging issues with responsiveness will be easier than trying to find the source of memory leaks and orphaned resources.

Archmonger commented 11 months ago

In the scenario where we auto-teardown, we've caught the error and can log the LOC. But in the scenario of user failure, it's a complete freeze with no user information.

Maybe we can default it to something large like 20 seconds? At that point we'd be 98% confident it's a programming failure and worth an ERROR log.

Archmonger commented 11 months ago

I'm realizing sync effects can be generators too.

Maybe we can stick with one use_effect but remove the ability to return a cancel function?

Archmonger commented 11 months ago

In terms of whether we diverge to creating use_async_effect, this issue is also relevant: https://github.com/reactive-python/reactpy/issues/1136

In my mind, threading should be unique to sync calls.

Archmonger commented 3 weeks ago

Someone already thought of these issues and developed something relatively clean in the ReactJS space.

Ref: https://marmelab.com/blog/2023/01/11/use-async-effect-react.html#introducing-the-code-classlanguage-textuseasynceffectcode-hook