better async effects

[rmorshea]

trafficstars

_{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

• [x] Tests have been included for all bug fixes or added functionality.
• [x] The changelog.rst has been updated with any significant changes.

[Archmonger]

Will review on Monday. Not home at the moment.

[rmorshea]

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]

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]

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

[rmorshea]

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]

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]

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]

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.