Repository navigation
Conversation
…fects A flush starts effects in priority order but doesn't wait for their async parts. An async `on_restore` callback that awaits (as shinychat's do) let the session's other effects and outputs start with the state not yet restored, so they ran once with default values. While the callbacks run, the session's effects with a lower priority than the restore effect now wait for them before their bodies run. The wait is per session, so other sessions keep running.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Async bookmark
on_restorecallbacks must finish before the session's other effects and outputs run. With the flush from #2508, those effects started early and saw state that was not restored yet. This PR makes them wait again.Refs #2508, for its TODO "Ordering for async bookmark
on_restore(priority 1000000)". Stacked on #2522.Summary
On
main, the flush awaited each effect in turn. Theon_restoreeffect has priority 1000000, so its callbacks finished (awaits included) before any other effect started. With #2508, the flush starts each effect in priority order but does not wait for its async part. When anon_restorecallback awaits, the session's other effects start first. They run once with default values, and then again after the restore. shinychat'senable_bookmarking()registers callbacks that await, so chat apps get this problem.This PR adds a gate for each session:
shiny/bookmark/_bookmark.py).shiny/reactive/_reactives.py). The effects still start in priority order. They keep the session busy, so no outputs go out before the restore is complete.Session._restore_gate()connects the two, andSessionProxysends module effects to the root session (shiny/session/_session.py).Review notes
main, where they ran before the restore effect.set_priority()takes effect on the next invalidation, so a changed priority cannot skip the gate.inithandler before the first flush. I did not use it, because it blocks the session's receive loop during the restore.onRestore()callbacks are synchronous._restore_gate(), in the same way that it needs_otel_reactive_update_span(). The mock intests/pytest/test_poll.pyhas one now.Testing
tests/pytest/test_concurrency.py. They cover the first output and effect values, priority order, effects above priority 1000000, other sessions, a callback that raises, a cancelled callback, the session end during a restore, module effects, a waiting effect that is destroyed, andset_priority(). Without the fix, 9 fail. The other one guards the priority 1000000 exception. I also removed theSessionProxydelegation, thefinally, and theasyncio.shield()one at a time, and each change made its test fail.check-format,check-lint, and pyright are clean. pyrefly reports 0 errors.