Repository navigation
fix(event): send a failed handler's writes after its last yield - #7696
FarhanAliRaza wants to merge 2 commits into
Conversation
A foreground handler that is cancelled or raises could leave changes it made after its last yield unsent. Memory and disk carried them in the next handler's delta, but a state manager that loads a fresh copy, as redis does, dropped the dirty marks, so the frontend never received them until the var changed again or the page reloaded. Flush the delta under the state lock before the exception propagates. A flush failure is logged so the handler's own exception still propagates.
[Medium impact] The PR appears safe to merge, with the existing non-blocking cancellation-test concern still open. Findings
SummarySends state changes made by a foreground handler before it fails or is cancelled.
Reviews (2) · Last reviewed commit: "Merge remote-tracking branch 'upstream/m..." · Reviewed by Greptile |
| future.cancel() | ||
| with contextlib.suppress(BaseException): | ||
| await asyncio.wait_for(future.wait_all(), timeout=5) |
There was a problem hiding this comment.
Cancellation test skips completion
future.cancel() has already completed the future, so wait_all() raises immediately instead of waiting for the running handler. Leaving the processor context then starts shutdown, which can cancel the task again if its flush suspends. The test therefore does not establish that normal cancellation sends cleanup before shutdown.
Wait for a handler-completion signal or a following event before leaving the context, then check the delta.
Merging this PR will not alter performance1 warning
Comparing |
…on-failed-handler # Conflicts: # tests/units/reflex_base/event/processor/test_base_state_processor.py
Related to #7248. Pairs with #7412.
Problem
A foreground handler can change state after its last
yield, for example in anexcept asyncio.CancelledErrorblock when asupersedes=Truecall cancels it, or just before it raises. Nothing sends those changes to the frontend.REFLEX_OPLOCK_ENABLED=False) never sends them. The next handler unpickles a fresh copy, and_init_bookkeepingclears the dirty marks. The frontend shows the old value until the var changes again or the page reloads.Fix
When
process_eventraises or is cancelled,_execute_eventflushes the delta withchain_updates(None, root_state=...)before the exception propagates. The flush runs under the state lock, so it reaches the frontend before any delta from the next handler. If the flush fails, it is logged and the handler's own exception propagates. The background branch uses the same pattern.Background handlers are not changed. They already flush in their
finally.Tests
test_failed_handler_emits_writes_after_last_yield: memory, disk and redis, each cancelled and raising. All 6 cases fail on main and pass with the fix.test_failed_handler_flush_error_keeps_handler_exception: a computed var makes the flush fail. The handler'sRuntimeErroris reported and the flush error is logged. It fails when theexceptin_flush_failed_handleris removed.Notes
docs/events/yield_events.mdabout this late delivery. Remove that sentence after this PR merges.