-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
rx.background and StateManager.modify_state provides safe exclusive access to state #1676
Conversation
App.modify_state provides exclusive access to state AND sends a state update after exiting the async contextmanager
…ations beat a race condition where the lock is held, but gets dropped before we can subscribe to notifications for the del key event
When encountering a background task EventHandler, move processing to an asyncio task and yield a final state update to unblock other client events. Background tasks do not get direct access to the State, but instead get `self` as a `StateProxy`, which protects against mutation of the vars. The `StateProxy` exposes async contextmanager protocol which refreshes the internal proxied instance from the state_manager and holds an exclusive lock on the token state while in context, allowing safe, stable reads and writes of state vars from a long running background process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, working really well for me! I think the async with self
syntax is nice. I didn't review the state.py
file yet, will add comments to that
improve the annotation and semantics of calling _process_background with a non-backgroundable event
Provide key expiration for `_redis_lock` operation. Raise LockExpiredError when trying to commit state after the lock expires. Use class-level constants for redis keyspace notification options and events that indicate a lock for a given key was released. Use keyspace notifications for the particular lock key we're waiting on, rather than keyevent notifications for all deleted or expired keys.
avoid sharing locks between instances of StateManager
It's easier to test if we're not blocking for minimum 1 second
This isn't ideal, but needed for py3.8 and py3.9 to work correctly.
Cloudpickle cannot work with states defined inside functions, they must be defined in an actual module.
Update tests to work when the StateManager is backed by an actual redis instance
github actions can be kinda slow, so give us some more wiggling room
Try to avoid CI flakiness by waiting much longer for lock expiry when running in CI
allows ImmutableMutableProxy to carry its immutable status
It's right next to the definition of rx.background itself and only defined once, then used within the State.
Inline with previously added MutableProxy and ImmutableMutableProxy
Avoid mixing logic between two different StateManager implementations. Fix all tests to identify StateManager type based on these classes. Update all integration (AppHarness) tests to work with redis. Replace emit_state_update with get_state, set_state, and modify_state functions on AppHarness.
@picklelo thanks for the feedback. I addressed almost everything here: https://github.com/reflex-dev/reflex/pull/1676/files/f6ba2ab4eb855837ae517e05ffc187765bbaaa93..1093229068b6d7f5aec2917c5d08b0ad43fb4211 I also went ahead and fixed all of the As for As an aside, I had to kick the CI on reflex-web a few times; I wouldn't think it's my changes here, but the error is weird and I don't really understand it. Edit: same error on a different PR that's already merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Further testing has revealed that this sort of breaks yielding updates in normal event handlers. Need to write a test case and investigate.
Fixed and test case written ✔️
This allows the update to be sent to the frontend, even while a (non-async) event handler is making a blocking call.
REDIS_URL is set in the integration test workflow; if we remove it here in the second branch of a session-scoped fixture, it accidentally disables redis for ALL remaining tests that follow it, even though it _seems_ like they are running with redis.
This reverts commit c76e0ee.
replace split with partition, which has more predictable behavior (always returns a 3-tuple)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!!
@@ -89,7 +97,7 @@ class App(Base): | |||
state: Type[State] = DefaultState | |||
|
|||
# Class to manage many client states. | |||
state_manager: StateManager = StateManager() | |||
state_manager: StateManager = StateManagerMemory(state=DefaultState) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we set this if we're overriding it again in the __init__
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to make it Optional
and default it to None
because then we have to put is not None
checks all over the place.
But if we do something like StateManager.create(state=DefaultState)
here, and redis is used, then we create an unserializable default, which pydantic doesn't like.
So the default StateManager
is the StateManagerMemory
, then inside __init__
, we call StateManager.create(...)
to update the reference to either memory or redis.
|
||
def get_state(self, token: str) -> State: | ||
|
||
class StateManagerMemory(StateManager): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, this is much cleaner we can extend to other backends in the future too
App.modify_state provides exclusive access to state AND sends a state update after exiting the async contextmanager.
Background EventHandler example
@rx.background
enables an EventHandler to not block the event loop!A background task can either yield events that will update the state or enter
async with self
context block to directly munge the state.Background task example app
Run it with Redis?
Bugs
Click Increment counter about 10 times quickly, then click Start Background Task... then it deadlocks 😔Fixed via b471403Remaining Work
StateProxy
and background events