Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix script runner stack overflow #8100

Merged
merged 2 commits into from Feb 8, 2024

Conversation

AnOctopus
Copy link
Collaborator

@AnOctopus AnOctopus commented Feb 6, 2024

Describe your changes

Convert script runner handling of rerun requests to use a while loop instead of recursion, to fix a stack overflow for scripts that infinitely rerun themselves.

GitHub Issue Link (if applicable)

Brought up in #7768

Testing Plan

It could probably be tested with threading.excepthook but I'm not sure that adds very much.


Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

Copy link
Collaborator

@LukasMasuch LukasMasuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The git diff makes this very hard to review. The only diff is the usage of while True instead of the self._run_script(rerun_exception_data) recursion, or? In that case, LGTM 👍

pages.values(),
),
None,
while True:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe a comment here would be nice why we need/use a while True loop here.

@AnOctopus
Copy link
Collaborator Author

Yep, it's just while True and converting the recursive call into a variable reassignment or a break, as appropriate

@AnOctopus AnOctopus merged commit f7b1337 into streamlit:develop Feb 8, 2024
37 checks passed
@AnOctopus AnOctopus deleted the fix/stack-overflow branch February 8, 2024 19:16
zyxue pushed a commit to zyxue/streamlit that referenced this pull request Apr 16, 2024
## Describe your changes
Convert script runner handling of rerun requests to use a while loop
instead of recursion, to fix a stack overflow for scripts that
infinitely rerun themselves.

## GitHub Issue Link (if applicable)
Brought up in streamlit#7768 

## Testing Plan

It could probably be tested with `threading.excepthook` but I'm not sure
that adds very much.

---

**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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants