allow state mutation with contextmanager from normal (non-background) events #5938
Conversation
…n-background events
CodSpeed Performance ReportMerging #5938 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Adds optional config to allow async with self syntax in normal (non-background) event handlers, enabling code reuse between background tasks and regular event handlers.
Key changes:
- Added new environment variable
REFLEX_STATE_ALLOW_CONTEXTMANAGER_WITHOUT_BACKGROUND_TASK(defaults toFalse) - Modified
BaseState.__aenter__to returnselfdirectly when config is enabled, instead of raisingTypeError - The context manager becomes a no-op in normal event handlers (which already hold state locks), maintaining safety while allowing syntax compatibility
Confidence Score: 5/5
- This PR is safe to merge with minimal risk
- The implementation is sound and addresses a valid use case. The changes are minimal, well-scoped, and maintain backward compatibility by defaulting to
False. The approach is safe because normal event handlers already hold state locks, making the context manager a harmless no-op. No breaking changes or risky patterns introduced. - No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| reflex/environment.py | 5/5 | Added new config option REFLEX_STATE_ALLOW_CONTEXTMANAGER_WITHOUT_BACKGROUND_TASK with default value False |
| reflex/state.py | 5/5 | Modified __aenter__ to allow async with self in normal event handlers when config option is enabled, returns self directly as a no-op |
Sequence Diagram
sequenceDiagram
participant EventHandler as Normal Event Handler
participant BgTask as Background Task
participant BaseState as BaseState
participant StateProxy as StateProxy
participant Config as Environment Config
Note over EventHandler,StateProxy: Before this PR
EventHandler->>BaseState: async with self
BaseState->>BaseState: __aenter__()
BaseState-->>EventHandler: TypeError: Only background task should use async with self
BgTask->>StateProxy: async with self
StateProxy->>StateProxy: __aenter__() - acquires lock
StateProxy-->>BgTask: StateProxy (mutable)
BgTask->>StateProxy: modify state
BgTask->>StateProxy: __aexit__()
StateProxy->>StateProxy: releases lock, emits update
Note over EventHandler,StateProxy: After this PR (config enabled)
EventHandler->>BaseState: async with self
BaseState->>Config: check ALLOW_CONTEXTMANAGER config
Config-->>BaseState: True
BaseState-->>EventHandler: self (no-op, lock already held)
EventHandler->>BaseState: modify state
EventHandler->>BaseState: __aexit__() (no-op)
BgTask->>StateProxy: async with self
Note over StateProxy: Unchanged behavior
StateProxy->>StateProxy: __aenter__() - acquires lock
StateProxy-->>BgTask: StateProxy (mutable)
2 files reviewed, no comments
masenf
left a comment
There was a problem hiding this comment.
yeah lets bring it without the config option. this is fine
|
one other thing to keep in mind is that |
I hoped for that answer
That's true, but I found that in many use cases that's not really an issue |
While reading this again: I was aware of that, but never really thought deeply about this. Is there a simple way to avoid the implicit |
|
the implicit yield is an implementation detail of using But i'm inclined to paper over that, with maybe a TODO comment for now, since i'm planning to refactor the event processing pipeline this weekend and will make it handle this case, and possibly even make the implicit yield an option that can be disabled. |
this makes it much easier to write reusable code which can be used in background and "normal" events and avoids a lot of dirty workarounds to make it possible
we can also choose to make this the default instead of adding just another config option