From a0527a5af598d78a0948c5316a405065e6085a71 Mon Sep 17 00:00:00 2001 From: Peter Law Date: Sat, 29 Jun 2024 21:41:21 +0100 Subject: [PATCH 1/4] Pass through the inference state id rather than recomputing it This removes some of the coupling between the management of the underlying process and the inference state itself, which intends to enable changing the origin of the id. This will be useful in the next commit. --- jedi/inference/compiled/subprocess/__init__.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/jedi/inference/compiled/subprocess/__init__.py b/jedi/inference/compiled/subprocess/__init__.py index cd5fe74c..76dba694 100644 --- a/jedi/inference/compiled/subprocess/__init__.py +++ b/jedi/inference/compiled/subprocess/__init__.py @@ -128,7 +128,7 @@ class InferenceStateSubprocess(_InferenceStateProcess): self._used = True result = self._compiled_subprocess.run( - self._inference_state_weakref(), + self._inference_state_id, func, args=args, kwargs=kwargs, @@ -213,18 +213,18 @@ class CompiledSubprocess: t) return process - def run(self, inference_state, function, args=(), kwargs={}): + def run(self, inference_state_id, function, args=(), kwargs={}): # Delete old inference_states. while True: try: - inference_state_id = self._inference_state_deletion_queue.pop() + delete_id = self._inference_state_deletion_queue.pop() except IndexError: break else: - self._send(inference_state_id, None) + self._send(delete_id, None) assert callable(function) - return self._send(id(inference_state), function, args, kwargs) + return self._send(inference_state_id, function, args, kwargs) def get_sys_path(self): return self._send(None, functions.get_sys_path, (), {}) From fff6e0ce2e528342af1cde980a82af9360a3de3a Mon Sep 17 00:00:00 2001 From: Peter Law Date: Sun, 30 Jun 2024 15:40:33 +0100 Subject: [PATCH 2/4] Drop unused member I'm not sure where this was used in the past, however it appears to be unused now. Removing this simplifies a change I'm about to make to _InferenceStateProcess. --- jedi/inference/compiled/subprocess/__init__.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/jedi/inference/compiled/subprocess/__init__.py b/jedi/inference/compiled/subprocess/__init__.py index 76dba694..3827e0e9 100644 --- a/jedi/inference/compiled/subprocess/__init__.py +++ b/jedi/inference/compiled/subprocess/__init__.py @@ -284,9 +284,6 @@ class CompiledSubprocess: class Listener: def __init__(self): self._inference_states = {} - # TODO refactor so we don't need to process anymore just handle - # controlling. - self._process = _InferenceStateProcess(Listener) def _get_inference_state(self, function, inference_state_id): from jedi.inference import InferenceState From 9d18b7c36d72685cb41127e223f0152f7f16a426 Mon Sep 17 00:00:00 2001 From: Peter Law Date: Sun, 30 Jun 2024 16:50:00 +0100 Subject: [PATCH 3/4] Document how Jedi manages its subprocesses This is derived from my understanding of the code, plus a bit of experimentation. --- jedi/api/environment.py | 15 ++- .../inference/compiled/subprocess/__init__.py | 115 ++++++++++++++++-- 2 files changed, 119 insertions(+), 11 deletions(-) diff --git a/jedi/api/environment.py b/jedi/api/environment.py index 75891799..278899aa 100644 --- a/jedi/api/environment.py +++ b/jedi/api/environment.py @@ -8,6 +8,7 @@ import hashlib import filecmp from collections import namedtuple from shutil import which +from typing import TYPE_CHECKING from jedi.cache import memoize_method, time_cache from jedi.inference.compiled.subprocess import CompiledSubprocess, \ @@ -15,6 +16,10 @@ from jedi.inference.compiled.subprocess import CompiledSubprocess, \ import parso +if TYPE_CHECKING: + from jedi.inference import InferenceState + + _VersionInfo = namedtuple('VersionInfo', 'major minor micro') # type: ignore[name-match] _SUPPORTED_PYTHONS = ['3.12', '3.11', '3.10', '3.9', '3.8', '3.7', '3.6'] @@ -102,7 +107,10 @@ class Environment(_BaseEnvironment): version = '.'.join(str(i) for i in self.version_info) return '<%s: %s in %s>' % (self.__class__.__name__, version, self.path) - def get_inference_state_subprocess(self, inference_state): + def get_inference_state_subprocess( + self, + inference_state: 'InferenceState', + ) -> InferenceStateSubprocess: return InferenceStateSubprocess(inference_state, self._get_subprocess()) @memoize_method @@ -134,7 +142,10 @@ class SameEnvironment(_SameEnvironmentMixin, Environment): class InterpreterEnvironment(_SameEnvironmentMixin, _BaseEnvironment): - def get_inference_state_subprocess(self, inference_state): + def get_inference_state_subprocess( + self, + inference_state: 'InferenceState', + ) -> InferenceStateSameProcess: return InferenceStateSameProcess(inference_state) def get_sys_path(self): diff --git a/jedi/inference/compiled/subprocess/__init__.py b/jedi/inference/compiled/subprocess/__init__.py index 3827e0e9..85291049 100644 --- a/jedi/inference/compiled/subprocess/__init__.py +++ b/jedi/inference/compiled/subprocess/__init__.py @@ -5,6 +5,23 @@ goals: 1. Making it safer - Segfaults and RuntimeErrors as well as stdout/stderr can be ignored and dealt with. 2. Make it possible to handle different Python versions as well as virtualenvs. + +The architecture here is briefly: + - For each Jedi `Environment` there is a corresponding subprocess which + operates within the target environment. If the subprocess dies it is replaced + at this level. + - `CompiledSubprocess` manages exactly one subprocess and handles communication + from the parent side. + - `Listener` runs within the subprocess, processing each request and yielding + results. + - `InterpreterEnvironment` provides an API which matches that of `Environment`, + but runs functionality inline rather than within a subprocess. It is thus + used both directly in places where a subprocess is unnecessary and/or + undesirable and also within subprocesses themselves. + - `InferenceStateSubprocess` (or `InferenceStateSameProcess`) provide high + level access to functionality within the subprocess from within the parent. + Each `InterpreterState` has an instance of one of these, provided by its + environment. """ import collections @@ -16,6 +33,7 @@ import traceback import weakref from functools import partial from threading import Thread +from typing import Dict, TYPE_CHECKING from jedi._compatibility import pickle_dump, pickle_load from jedi import debug @@ -25,6 +43,9 @@ from jedi.inference.compiled.access import DirectObjectAccess, AccessPath, \ SignatureParam from jedi.api.exceptions import InternalError +if TYPE_CHECKING: + from jedi.inference import InferenceState + _MAIN_PATH = os.path.join(os.path.dirname(__file__), '__main__.py') PICKLE_PROTOCOL = 4 @@ -83,10 +104,10 @@ def _cleanup_process(process, thread): class _InferenceStateProcess: - def __init__(self, inference_state): + def __init__(self, inference_state: 'InferenceState') -> None: self._inference_state_weakref = weakref.ref(inference_state) self._inference_state_id = id(inference_state) - self._handles = {} + self._handles: Dict[int, AccessHandle] = {} def get_or_create_access_handle(self, obj): id_ = id(obj) @@ -116,7 +137,24 @@ class InferenceStateSameProcess(_InferenceStateProcess): class InferenceStateSubprocess(_InferenceStateProcess): - def __init__(self, inference_state, compiled_subprocess): + """ + API to functionality which will run in a subprocess. + + This mediates the interaction between an `InferenceState` and the actual + execution of functionality running within a `CompiledSubprocess`. Available + functions are defined in `.functions`, though should be accessed via + attributes on this class of the same name. + + This class is responsible for indicating that the `InferenceState` within + the subprocess can be removed once the corresponding instance in the parent + goes away. + """ + + def __init__( + self, + inference_state: 'InferenceState', + compiled_subprocess: 'CompiledSubprocess', + ) -> None: super().__init__(inference_state) self._used = False self._compiled_subprocess = compiled_subprocess @@ -164,6 +202,17 @@ class InferenceStateSubprocess(_InferenceStateProcess): class CompiledSubprocess: + """ + A subprocess which runs inference within a target environment. + + This class manages the interface to a single instance of such a process as + well as the lifecycle of the process itself. See `.__main__` and `Listener` + for the implementation of the subprocess and details of the protocol. + + A single live instance of this is maintained by `jedi.api.environment.Environment`, + so that typically a single subprocess is used at a time. + """ + is_crashed = False def __init__(self, executable, env_vars=None): @@ -272,16 +321,59 @@ class CompiledSubprocess: def delete_inference_state(self, inference_state_id): """ - Currently we are not deleting inference_state instantly. They only get - deleted once the subprocess is used again. It would probably a better - solution to move all of this into a thread. However, the memory usage - of a single inference_state shouldn't be that high. + Indicate that an inference state (in the subprocess) is no longer + needed. + + The state corresponding to the given id will become inaccessible and the + id may safely be re-used to refer to a different context. + + Note: it is not guaranteed that the corresponding state will actually be + deleted immediately. """ - # With an argument - the inference_state gets deleted. + # Currently we are not deleting the related state instantly. They only + # get deleted once the subprocess is used again. It would probably a + # better solution to move all of this into a thread. However, the memory + # usage of a single inference_state shouldn't be that high. self._inference_state_deletion_queue.append(inference_state_id) class Listener: + """ + Main loop for the subprocess which actually does the inference. + + This class runs within the target environment. It listens to instructions + from the parent process, runs inference and returns the results. + + The subprocess has a long lifetime and is expected to process several + requests, including for different `InferenceState` instances in the parent. + See `CompiledSubprocess` for the parent half of the system. + + Communication is via pickled data sent serially over stdin and stdout. + Stderr is read only if the child process crashes. + + The request protocol is a 4-tuple of: + * inference_state_id | None: an opaque identifier of the parent's + `InferenceState`. An `InferenceState` operating over an + `InterpreterEnvironment` is created within this process for each of + these, ensuring that each parent context has a corresponding context + here. This allows context to be persisted between requests. Unless + `None`, the local `InferenceState` will be passed to the given function + as the first positional argument. + * function | None: the function to run. This is expected to be a member of + `.functions`. `None` indicates that the corresponding inference state is + no longer needed and should be dropped. + * args: positional arguments to the `function`. If any of these are + `AccessHandle` instances they will be adapted to the local + `InferenceState` before being passed. + * kwargs: keyword arguments to the `function`. If any of these are + `AccessHandle` instances they will be adapted to the local + `InferenceState` before being passed. + + The result protocol is a 3-tuple of either: + * (False, None, function result): if the function returns without error, or + * (True, traceback, exception): if the function raises an exception + """ + def __init__(self): self._inference_states = {} @@ -345,7 +437,12 @@ class Listener: class AccessHandle: - def __init__(self, subprocess, access, id_): + def __init__( + self, + subprocess: _InferenceStateProcess, + access: DirectObjectAccess, + id_: int, + ) -> None: self.access = access self._subprocess = subprocess self.id = id_ From a67deeb60270c4d458b3cebde276ca51e7657aa4 Mon Sep 17 00:00:00 2001 From: Peter Law Date: Sun, 30 Jun 2024 17:16:47 +0100 Subject: [PATCH 4/4] Fix race condition around subprocess inference state tidyup There was a race condition due to the combination of Python's object ids being re-usable and Jedi persisting such ids beyond the real lifeteime of some objects. This could lead to the subprocess' view of the lifetime of `InferenceState` contexts getting out of step with that in the parent process and resulting in errors when removing them. It is also possible that this could result in erroneous results being reported, however this was not directly observed. The race was specifically: - `InferenceState` A created, gets id 1 - `InferenceStateSubprocess` A' created, uses `InferenceState` A which it stores as a weakref and an id - `InferenceStateSubprocess` A' is used, the sub-process learns about an `InferenceState` with id 1 - `InferenceState` A goes away, `InferenceStateSubprocess` A' is not yet garbage collected - `InferenceState` B created, gets id 1 - `InferenceStateSubprocess` B' created, uses `InferenceState` B which it stores as a weakref and an id - `InferenceStateSubprocess` B' is used, the sub-process re-uses its entry for an `InferenceState` with id 1 At this point the order of operations between the two `InferenceStateSubprocess` instances going away is immaterial -- both will trigger a removal of a state with id 1. As long as B' doesn't try to use the sub-process again after the first removal has happened then the second removal will fail. This commit resolves the race condition by coupling the context in the subprocess to the corresponding manager class instance in the parent process, rather than to the consumer `InferenceState`. See inline comments for further details. --- .../inference/compiled/subprocess/__init__.py | 29 ++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/jedi/inference/compiled/subprocess/__init__.py b/jedi/inference/compiled/subprocess/__init__.py index 85291049..3a6039f7 100644 --- a/jedi/inference/compiled/subprocess/__init__.py +++ b/jedi/inference/compiled/subprocess/__init__.py @@ -106,7 +106,6 @@ def _cleanup_process(process, thread): class _InferenceStateProcess: def __init__(self, inference_state: 'InferenceState') -> None: self._inference_state_weakref = weakref.ref(inference_state) - self._inference_state_id = id(inference_state) self._handles: Dict[int, AccessHandle] = {} def get_or_create_access_handle(self, obj): @@ -159,6 +158,27 @@ class InferenceStateSubprocess(_InferenceStateProcess): self._used = False self._compiled_subprocess = compiled_subprocess + # Opaque id we'll pass to the subprocess to identify the context (an + # `InferenceState`) which should be used for the request. This allows us + # to make subsequent requests which operate on results from previous + # ones, while keeping a single subprocess which can work with several + # contexts in the parent process. Once it is no longer needed(i.e: when + # this class goes away), we also use this id to indicate that the + # subprocess can discard the context. + # + # Note: this id is deliberately coupled to this class (and not to + # `InferenceState`) as this class manages access handle mappings which + # must correspond to those in the subprocess. This approach also avoids + # race conditions from successive `InferenceState`s with the same object + # id (as observed while adding support for Python 3.13). + # + # This value does not need to be the `id()` of this instance, we merely + # need to ensure that it enables the (visible) lifetime of the context + # within the subprocess to match that of this class. We therefore also + # depend on the semantics of `CompiledSubprocess.delete_inference_state` + # for correctness. + self._inference_state_id = id(self) + def __getattr__(self, name): func = _get_function(name) @@ -330,6 +350,10 @@ class CompiledSubprocess: Note: it is not guaranteed that the corresponding state will actually be deleted immediately. """ + # Warning: if changing the semantics of context deletion see the comment + # in `InferenceStateSubprocess.__init__` regarding potential race + # conditions. + # Currently we are not deleting the related state instantly. They only # get deleted once the subprocess is used again. It would probably a # better solution to move all of this into a thread. However, the memory @@ -397,6 +421,9 @@ class Listener: if inference_state_id is None: return function(*args, **kwargs) elif function is None: + # Warning: if changing the semantics of context deletion see the comment + # in `InferenceStateSubprocess.__init__` regarding potential race + # conditions. del self._inference_states[inference_state_id] else: inference_state = self._get_inference_state(function, inference_state_id)