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: Provide a warning when running autocomplete in a way that could segfault #5954

Merged
merged 2 commits into from
Aug 23, 2024

Conversation

niloc132
Copy link
Member

@niloc132 niloc132 commented Aug 19, 2024

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 a set_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

if not version('numpy').startswith('2.0') or not version('jedi') == '0.19.1':
return

from . import MAX_RECURSION_LIMIT
Copy link
Member

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?

Copy link
Member Author

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.

@niloc132 niloc132 requested a review from chipkent August 22, 2024 16:05
@niloc132
Copy link
Member Author

After a video call with Jianfeng:

  • We're going to remove the numpy and jedi version check, so that we can just guard against jedi setting the recursion limit high for py 3.9 and 3.10
  • Initial import of jedi will be guarded with a check on this limit, and we'll lower the limit right away
  • The constant will be moved to _completer.py, but __init__.py will get a setter function (guarding against any future issue where the limit might need to be lower than I have already set it)

At some point I need to follow up by writing a smaller C program that loads libpython and shows the segfault

Copy link
Contributor

@jmao-denver jmao-denver left a 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?

@niloc132
Copy link
Member Author

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)?

@niloc132 niloc132 merged commit 76004db into deephaven:main Aug 23, 2024
17 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autocomplete seg faults when attempting to autocomplete np on same statement as import
4 participants