Add retry to watcher.util.path_modification_time to try and fix race condition. #10148
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
Also refactor retry code so it's easier to use.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
vdonato
approved these changes
Jan 10, 2025
vdonato
added
the
security-assessment-completed
kmcgrady
added
change:bugfix
edegp
pushed a commit
to edegp/streamlit
that referenced
this pull request
Jan 19, 2025
…condition. (streamlit#10148) Add retry to watcher.util.path_modification_time to **try** and fix race condition. I have no guarantee this fixes it, but from the error message it seems right. And it doesn't hurt anyway. ## Describe your changes I just added some retries around an `os.stat(path).st_mtime` that could otherwise lead to a race condition:  I think this race condition is more likely to happen when you have tooling that runs on save. In my case, I have a linter/formatter that fixes issues and saves the file again. So sometimes when we get to the `os.stat(path).st_mtime` line the file is in the process of being modified, leading to the error above. BTW, as part of this work I also created two helper functions to do retries in the watcher code, since retries can be tricky. ## GitHub Issue Link (if applicable) ## Testing Plan - ~~Explanation of why no additional tests are needed~~ - Unit Tests (JS and/or Python): **None**. I am unable to reproduce the original problem. But the watcher code is well-tested enough that I'm confident this at least doesn't break anything. - E2E Tests: **None** - Any manual testing needed? **Yes**. Please try modifying your Streamlit app on Mac, Linux, Windows, using different editors (VS Code, PyCharm, etc). Even better if you can turn on auto-formatting plugins as well. --- **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.
Add retry to watcher.util.path_modification_time to try and fix race condition. I have no guarantee this fixes it, but from the error message it seems right. And it doesn't hurt anyway.
Describe your changes
I just added some retries around an
os.stat(path).st_mtimethat could otherwise lead to a race condition:I think this race condition is more likely to happen when you have tooling that runs on save. In my case, I have a linter/formatter that fixes issues and saves the file again. So sometimes when we get to the
os.stat(path).st_mtimeline the file is in the process of being modified, leading to the error above.BTW, as part of this work I also created two helper functions to do retries in the watcher code, since retries can be tricky.
GitHub Issue Link (if applicable)
Testing Plan
Explanation of why no additional tests are neededContribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.