From d46098994fee03f52f4d054550f663f4ba563463 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Mon, 19 Aug 2024 10:19:47 -0500 Subject: [PATCH 1/2] Provide a warning when running autocomplete in a way that could segfault --- .../auto_completer/__init__.py | 8 ++++ .../auto_completer/_completer.py | 38 +++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/py/server/deephaven_internal/auto_completer/__init__.py b/py/server/deephaven_internal/auto_completer/__init__.py index ffb6eea0888..7d6dac924b6 100644 --- a/py/server/deephaven_internal/auto_completer/__init__.py +++ b/py/server/deephaven_internal/auto_completer/__init__.py @@ -19,6 +19,14 @@ from ._completer import Completer, Mode from jedi import preload_module, Interpreter + +""" +For Python 3.9 and 3.10, there is a bug in recursion which can result in a segfault. Lowering this +limit to 2000 or less seems to mitigate it. +""" +MAX_RECURSION_LIMIT = 2000 + + jedi_settings = Completer() # warm jedi up a little. We could probably off-thread this. preload_module("deephaven") diff --git a/py/server/deephaven_internal/auto_completer/_completer.py b/py/server/deephaven_internal/auto_completer/_completer.py index f87c3c0b56c..99ead0d1b67 100644 --- a/py/server/deephaven_internal/auto_completer/_completer.py +++ b/py/server/deephaven_internal/auto_completer/_completer.py @@ -6,6 +6,9 @@ from typing import Any, Union, List from jedi import Interpreter, Script from jedi.api.classes import Completion, Signature +from importlib.metadata import version +import sys +import warnings class Mode(Enum): @@ -80,6 +83,7 @@ def __init__(self): except ImportError: self.__can_jedi = False self.mode = Mode.OFF + self.recursion_limit_already_warned = False @property def mode(self) -> Mode: @@ -135,6 +139,7 @@ def do_completion( Modeled after Jedi language server https://github.com/pappasam/jedi-language-server/blob/main/jedi_language_server/server.py#L189 """ + self.check_recursion_limit() if not self._versions[uri] == version: # if you aren't the newest completion, you get nothing, quickly return [] @@ -253,3 +258,36 @@ def do_hover( hoverstring += '\n---\n' + wrap_plaintext(raw_docstring) return hoverstring.strip() + + def check_recursion_limit(self): + """ + Tests for python+jedi+numpy versions that are susceptible to a RecursionError/segfault issue, and lowers + the recursion limit, warning if the limit is raised externally + """ + if not sys.version.startswith('3.9.') and not sys.version.startswith('3.10.'): + return + + if not self.__can_jedi: + return + + # numpy is a required dependency, tested for jedi above + if not version('numpy').startswith('2.0') or not version('jedi') == '0.19.1': + return + + from . import MAX_RECURSION_LIMIT + + if sys.getrecursionlimit() <= MAX_RECURSION_LIMIT: + return + + sys.setrecursionlimit(MAX_RECURSION_LIMIT) + + if not self.recursion_limit_already_warned: + self.recursion_limit_already_warned = True + warnings.warn(f"""Recursion limit has been set to {MAX_RECURSION_LIMIT} to avoid a known segfault in Python related to RecursionErrors. +This limit will be set to {MAX_RECURSION_LIMIT} whenever autocomplete takes place to avoid this. Other steps you can +take to avoid this: + * Use numpy 1.x + * Use Python 3.8 or 3.11+ + * When available, use a newer version of Jedi + * Disable autocomplete +See https://github.com/deephaven/deephaven-core/issues/5878 for more information.""") From 9814ac6fc3ddd7d061d9f580ce30c4f76609cada Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Fri, 23 Aug 2024 08:29:10 -0500 Subject: [PATCH 2/2] Review feedback --- .../auto_completer/__init__.py | 15 +++---- .../auto_completer/_completer.py | 39 +++++++++---------- 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/py/server/deephaven_internal/auto_completer/__init__.py b/py/server/deephaven_internal/auto_completer/__init__.py index 7d6dac924b6..8ba39520d27 100644 --- a/py/server/deephaven_internal/auto_completer/__init__.py +++ b/py/server/deephaven_internal/auto_completer/__init__.py @@ -20,14 +20,15 @@ from jedi import preload_module, Interpreter -""" -For Python 3.9 and 3.10, there is a bug in recursion which can result in a segfault. Lowering this -limit to 2000 or less seems to mitigate it. -""" -MAX_RECURSION_LIMIT = 2000 - - jedi_settings = Completer() # warm jedi up a little. We could probably off-thread this. preload_module("deephaven") Interpreter("", []).complete(1, 0) + + +def set_max_recursion_limit(limit: int) -> None: + """ + Utility method to raise/lower the limit that our autocompletion will impose on Python 3.9 and 3.10. + """ + from . import _completer + _completer.MAX_RECURSION_LIMIT = limit diff --git a/py/server/deephaven_internal/auto_completer/_completer.py b/py/server/deephaven_internal/auto_completer/_completer.py index 99ead0d1b67..39ea08cc25a 100644 --- a/py/server/deephaven_internal/auto_completer/_completer.py +++ b/py/server/deephaven_internal/auto_completer/_completer.py @@ -39,6 +39,13 @@ def __str__(self) -> str: } +""" +For Python 3.9 and 3.10, there is a bug in recursion which can result in a segfault. Lowering this +limit to 2000 or less seems to mitigate it. +""" +MAX_RECURSION_LIMIT = 2000 + + def wrap_python(txt: str) -> str: """ Wraps a string in a Python fenced codeblock for markdown @@ -84,6 +91,7 @@ def __init__(self): self.__can_jedi = False self.mode = Mode.OFF self.recursion_limit_already_warned = False + self.check_recursion_limit(True) @property def mode(self) -> Mode: @@ -259,35 +267,26 @@ def do_hover( return hoverstring.strip() - def check_recursion_limit(self): + def check_recursion_limit(self, suppress_warning: bool = False) -> None: """ Tests for python+jedi+numpy versions that are susceptible to a RecursionError/segfault issue, and lowers - the recursion limit, warning if the limit is raised externally + the recursion limit, warning if the limit is raised externally. """ - if not sys.version.startswith('3.9.') and not sys.version.startswith('3.10.'): + if sys.version_info < (3, 9) or sys.version_info >= (3, 11): return - if not self.__can_jedi: - return - - # numpy is a required dependency, tested for jedi above - if not version('numpy').startswith('2.0') or not version('jedi') == '0.19.1': - return - - from . import MAX_RECURSION_LIMIT - if sys.getrecursionlimit() <= MAX_RECURSION_LIMIT: return sys.setrecursionlimit(MAX_RECURSION_LIMIT) - if not self.recursion_limit_already_warned: + # Log a warning if the user (or some user code) seems to have tried to raise the limit again after we lowered it. + # This is not a fool-proof way to keep the limit down, and isn't meant to be, only to guard against the primary + # way we've seen to cause this issue. + if not suppress_warning and not self.recursion_limit_already_warned: self.recursion_limit_already_warned = True - warnings.warn(f"""Recursion limit has been set to {MAX_RECURSION_LIMIT} to avoid a known segfault in Python related to RecursionErrors. -This limit will be set to {MAX_RECURSION_LIMIT} whenever autocomplete takes place to avoid this. Other steps you can -take to avoid this: - * Use numpy 1.x - * Use Python 3.8 or 3.11+ - * When available, use a newer version of Jedi - * Disable autocomplete + warnings.warn(f"""Recursion limit has been set to {MAX_RECURSION_LIMIT} to avoid a known segfault in Python 3.9 and 3.10 +related to RecursionErrors. This limit will be set to {MAX_RECURSION_LIMIT} whenever autocomplete takes place +to avoid this, because the jedi library sets this to 3000, above the safe limit. Disabling autocomplete +will prevent this check, as it will also prevent jedi from raising the limit. See https://github.com/deephaven/deephaven-core/issues/5878 for more information.""")