Retrieve pages for new session when ready to send the event #8702
Conversation
kmcgrady
force-pushed
the
fix/new-session-race-condition
branch
from
0414461
to
00820d1
Compare
kmcgrady
force-pushed
the
fix/new-session-race-condition
branch
from
00820d1
to
6f4bd1f
Compare
lib/streamlit/runtime/app_session.py
Outdated
| self, | ||
| page_script_hash: str, | ||
| fragment_ids_this_run: set[str] | None = None, | ||
| pages: dict[PageHash, PageInfo] = {}, |
There was a problem hiding this comment.
This may end up resulting in bugs down the line because the default pages dict is a single dict and not a new one that gets created every time a default value is needed, so if it's ever mutated the changes will persist the next time the default value is used.
It's unlikely that we'll mutate the default value here but likely don't want to write things this way anyway just to be safe.
There was a problem hiding this comment.
I don't think this PR totally removes the race condition -- it likely just drops the probability of it a lot so that it may be close to impossible to trigger, but in theory the issue still exists.
The problem is that st.navigate may be called by the user script in between the SCRIPT_STARTED event being sent by the script runner and the event being processed by _handle_scriptrunner_event_on_event_loop.
We'll want to call get_pages when the event is fired by the script runner to be sure this can't happen
lib/streamlit/runtime/app_session.py
Outdated
| @@ -502,6 +505,7 @@ def _handle_scriptrunner_event_on_event_loop( | |||
| client_state: ClientState | None = None, | |||
| page_script_hash: str | None = None, | |||
| fragment_ids_this_run: set[str] | None = None, | |||
| pages: dict[PageHash, PageInfo] = {}, | |||
There was a problem hiding this comment.
Same comment about the default value used here
There was a problem hiding this comment.
This one seems a bit weird to remove. I can do that here, but it's just another handler tied to the event handler. Will wait for your thoughts and make sure we align on this.
There was a problem hiding this comment.
The correct way of doing this would be to have the default value be None and initialize it to an empty dict in the function if necessary so that we don't have a single default dict shared by every call to the function that doesn't provide its own
There was a problem hiding this comment.
Ah. I gotcha here. I'll take a look!
lib/streamlit/runtime/app_session.py
Outdated
| @@ -474,6 +475,7 @@ def _on_scriptrunner_event( | |||
| client_state: ClientState | None = None, | |||
| page_script_hash: str | None = None, | |||
| fragment_ids_this_run: set[str] | None = None, | |||
| pages: dict[PageHash, PageInfo] = {}, | |||
There was a problem hiding this comment.
Same comment about the default value used here
There was a problem hiding this comment.
I can probably address the others. This one probably cannot cause of the way the event structure is worked. There's got to be a default for each one otherwise, every script runner event requires it (which seems over the top).
It perhaps suggests that having one event handler for any event going on in Streamlit might not be the most scalable.
Isn't that what this line does? Calling Or are you referring to the |
Whoops, I must have marked the one-line change in that file as reviewed early on and then forgotten about it when reading through the |
Describe your changes
We have found flaky situations where pages are set via
st.navigationbefore the NewSession call is sent. This causes the NewSession message to retrieve pages it should not be expecting from the PagesManager. The easy solution here is to send the pages on the Script Started Event so that the NewSession has the pages at the time of the event.I believe there's a larger discussion on how we should be managing this. Likely, we should ensure information is happening at the right moment.
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.