-
Notifications
You must be signed in to change notification settings - Fork 80
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: Provide a warning when running autocomplete in a way that could segfault #5954
Conversation
if not version('numpy').startswith('2.0') or not version('jedi') == '0.19.1': | ||
return | ||
|
||
from . import MAX_RECURSION_LIMIT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason that MAX_RECURSION_LIMIT
is not included in this file instead of in the root? Do you expect it to be used elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way it can easily be set by a user if they want to fine tune the limit further. Given that we don't understand why the issue is happening to begin with, it might help to let us tweak it at runtime without a new release.
After a video call with Jianfeng:
At some point I need to follow up by writing a smaller C program that loads libpython and shows the segfault |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a follow-up ticket to remove these extra code once the issue is no longer a threat as we bump the min Py version beyond the problematic ones or jedi fixes the issue on their end?
Jedi fixing the issue won't stop jedi from raising the recursion limit, so we need to keep the fix so that deeply recursive calls of any kind don't crash py 3.9 and 3.10. Python 3.10 is EOL'd end of 2026, so in about 2.5 years we would consider that ticket... do we want to keep it in the tracker that long? or just leave this as an innocuous fix (and i'll set a calendar reminder that far out)? |
Simple mitigation for jedi setting the recursion limit too high for Python 3.9 and 3.10 to correctly handle RecursionErrors. This prevents a segfault for a known bug in jedi, and only applies in cases where we know the bug can take place.
Technically it is possible for these python versions to crash in this way without autocomplete or numpy, but it first would require the recursion limit to be raised, so the fix only applies when autocomplete is used.
The
deephaven_internal.autocompleter
module has aset_max_recursion_limit
method to define a different max recursion limit for an application, in case the default of 2000 is not sufficient for all cases.Fixes #5878