better async effects #1169

rmorshea · 2023-12-09T18:46:42Z

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

Issues

Closes: #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 #1093

Checklist

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

Archmonger · 2023-12-10T06:31:25Z

Will review on Monday. Not home at the moment.

Archmonger

I like the flexibility of generator approach a lot more than our previously proposed stop_event interface.

But I do still think safeguards (timeout) need to be put in place for programmer error. We can't trust everyone to right perfect code, so we need to detect, warn, then mitigate programming flaws.

Also, now that our async function interface has deviated wildly from the sync equivalent, it's starting to become clear to me we shouldn't overload use_effect for async support any longer. We need to migrate towards a use_async_effect hook.

Archmonger · 2023-12-11T02:33:48Z

docs/source/reference/hooks-api.rst

+    If you never ``yield`` control back to ReactPy, then the component will never
+    re-render and the effect will never be cleaned up. This is a common mistake when
+    using ``use_effect`` for the first time.


I don't like the idea of having no safeguard against this. An interface that doesn't protect against critical breaking behavior is usually the wrong interface.

My intuition has been telling me we probably want a new use_async_effect hook that incorporates a timeout parameter. Since ReactJS useEffect only supports sync effects, it's a (minor) mismatch that we're overloading our equivalent with async support.

Archmonger · 2023-12-11T02:40:46Z

docs/source/reference/hooks-api.rst

 Here, a new connection will be established whenever a new ``url`` is set.

+.. warning::
+
+    A component will be unable to render until all its outstanding effects have been


render -> re-render

Archmonger · 2023-12-11T02:43:57Z

docs/source/reference/hooks-api.rst

+or an async generator. If your effect doesn't need to do any cleanup, then you can
+simply write an async function.


then you can simply write -> then you should use

Archmonger · 2023-12-11T02:48:56Z

src/py/reactpy/reactpy/core/_life_cycle_hook.py

-        self._effect_tasks.extend(create_task(e(stop)) for e in self._effect_funcs)
+        for effect_func in self._effect_funcs:
+            effect_task = create_task(effect_func(stop))
+            # potential memory leak if the task doesn't remove itself when complete


This comment needs a bit more description.

Is the memory leak due to our internal design, or due to user failure? Is the next LOC designed to prevent this issue, or is it the cause of this issue?

Archmonger · 2023-12-11T02:49:52Z

src/py/reactpy/reactpy/core/_life_cycle_hook.py

+            # potential memory leak if the task doesn't remove itself when complete
+            effect_task.add_done_callback(lambda t: self._effect_tasks.remove(t))
+            self._effect_tasks.add(effect_task)
+        self._effect_tasks.update()


Is there any reason to call update here?

Archmonger · 2023-12-11T02:51:59Z

src/py/reactpy/reactpy/core/hooks.py

+    def add_effect(function: _Effect) -> None:
+        effect_func = _cast_async_effect(function)
+
+        async def run_effect(stop: Event) -> None:


Does this function really need to be dynamically generated within add_effect? It would be more memory efficient to move run_effect outside of add_effect, and pass in all needed values via args.

Dynamically defined functions are generally a bad idea for any long-running Python code due to memory usage.

Archmonger · 2023-12-11T02:53:03Z

src/py/reactpy/reactpy/core/hooks.py

+                        f"Async effect {async_function_effect} returned a cleanup "
+                        f"function - use an async generator instead by yielding inside "
+                        "a try/finally block. This will be an error in a future "
+                        "version of ReactPy.",


This needs to be mentioned in the changelog.

Archmonger · 2023-12-11T02:54:35Z

docs/source/reference/hooks-api.rst

+Alternatively to the generator style of cleanup, you can return a cleanup function from
+your effect function. As with the generator style, the cleanup function will be run
+before the component is unmounted or before the next effect is triggered when the
+component re-renders:


The fact that returning cleanup functions will continue being allowed on sync effects (but not async effects) really makes me think we should commit to a use_async_effect hook.

It feels confusing to mandate two completely separate user interfaces within the same hook.

rmorshea · 2023-12-12T04:02:43Z

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 · 2023-12-12T04:28:06Z

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 · 2023-12-12T04:47:59Z

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

rmorshea · 2023-12-12T18:31:20Z

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 · 2023-12-12T22:45:40Z

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 · 2023-12-14T18:19:16Z

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 · 2023-12-17T23:38:23Z

In terms of whether we diverge to creating use_async_effect, this issue is also relevant: #1136

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

rmorshea added 3 commits December 9, 2023 11:38

better async effects

af162be

add changelog entry

773e8d4

Merge branch 'main' into better-async-effects

44328bb

rmorshea added this to the 1.1.0 milestone Dec 9, 2023

rmorshea added 3 commits December 9, 2023 12:00

3.9 compat annotations

bd5bb0c

fix strict equality tests

5efe2f6

Merge branch 'main' into better-async-effects

962b7f4

rmorshea requested a review from Archmonger December 10, 2023 06:30

Archmonger requested changes Dec 11, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

better async effects #1169

better async effects #1169

rmorshea commented Dec 9, 2023

Archmonger commented Dec 10, 2023

Archmonger left a comment •

edited

Loading

Archmonger Dec 11, 2023 •

edited

Loading

Archmonger Dec 11, 2023

Archmonger Dec 11, 2023

Archmonger Dec 11, 2023

Archmonger Dec 11, 2023

Archmonger Dec 11, 2023 •

edited

Loading

Archmonger Dec 11, 2023

Archmonger Dec 11, 2023 •

edited

Loading

rmorshea commented Dec 12, 2023

Archmonger commented Dec 12, 2023

Archmonger commented Dec 12, 2023

rmorshea commented Dec 12, 2023 •

edited

Loading

Archmonger commented Dec 12, 2023 •

edited

Loading

Archmonger commented Dec 14, 2023 •

edited

Loading

Archmonger commented Dec 17, 2023 •

edited

Loading

		or an async generator. If your effect doesn't need to do any cleanup, then you can
		simply write an async function.

better async effects #1169

Are you sure you want to change the base?

better async effects #1169

Conversation

rmorshea commented Dec 9, 2023

Issues

Solution

Checklist

Archmonger commented Dec 10, 2023

Archmonger left a comment • edited Loading

Choose a reason for hiding this comment

Archmonger Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Archmonger Dec 11, 2023

Choose a reason for hiding this comment

Archmonger Dec 11, 2023

Choose a reason for hiding this comment

Archmonger Dec 11, 2023

Choose a reason for hiding this comment

Archmonger Dec 11, 2023

Choose a reason for hiding this comment

Archmonger Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Archmonger Dec 11, 2023

Choose a reason for hiding this comment

Archmonger Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

rmorshea commented Dec 12, 2023

Archmonger commented Dec 12, 2023

Archmonger commented Dec 12, 2023

rmorshea commented Dec 12, 2023 • edited Loading

Archmonger commented Dec 12, 2023 • edited Loading

Archmonger commented Dec 14, 2023 • edited Loading

Archmonger commented Dec 17, 2023 • edited Loading

Archmonger left a comment •

edited

Loading

Archmonger Dec 11, 2023 •

edited

Loading

Archmonger Dec 11, 2023 •

edited

Loading

Archmonger Dec 11, 2023 •

edited

Loading

rmorshea commented Dec 12, 2023 •

edited

Loading

Archmonger commented Dec 12, 2023 •

edited

Loading

Archmonger commented Dec 14, 2023 •

edited

Loading

Archmonger commented Dec 17, 2023 •

edited

Loading