Don't request_rerun for fragments that do not exist anymore #10147
+49
−0
Conversation
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
raethlein
changed the title
raethlein
added
security-assessment-completed
raethlein
force-pushed
the
fix/issue-9921-3
branch
from
ddeb7f1 to
794b0f4
Compare
raethlein
force-pushed
the
fix/issue-9921-3
branch
from
794b0f4 to
4bb30b0
Compare
raethlein
changed the title
edegp
pushed a commit
to edegp/streamlit
that referenced
this pull request
Jan 19, 2025
…t#10147) ## Describe your changes We stopped raising a RuntimeError when a fragment with a specified id cannot be found. This solves the original issue in streamlit#9921, but surfaced a different issue where the dialog did not always close when the button in the dialog was clicked in fast succession. The reason for this is that in app_session, a fragment run might create a new ScriptRunner when the current ScriptRunner is in state STOPPED is false and the new ScriptRunner is created). This will lead to all events from the previous script runner being ignored. When the full app rerun ScriptRunner is done (STOPPED) but its events are not processed before the new ScriptRunner is created, its finished message is not sent to the frontend and no cleanup is happening. ## GitHub Issue Link (if applicable) ## Testing Plan - Unit Tests (JS and/or Python) - Add unit test to ensure that the scriptrunner is not requested if the fragment does not even exist anymore - E2E Tests - The e2e test that was added in streamlit#10132 _could_ catch this one here, although it happens more rarely than the race condition tackled in that PR --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
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.
Describe your changes
Related to #9921 and extracted from PR #9965
Related to #10132
In #10130, we stopped raising a RuntimeError when a fragment with a specified id cannot be found. This solves the original issue in #9921, but surfaced a different issue where the dialog did not always close when the button in the dialog was clicked in fast succession.
The reason for this is that in app_session, a fragment run might create a new ScriptRunner when the current ScriptRunner is in state STOPPED (in this case, success here is false and the new ScriptRunner is created). This will lead to all events from the previous script runner being ignored (see here). When the full app rerun ScriptRunner is done (STOPPED) but its events are not processed before the new ScriptRunner is created, its finished message is not sent to the frontend and no cleanup is happening.
GitHub Issue Link (if applicable)
Testing Plan
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.