mirror of
https://github.com/tiennm99/serena.git
synced 2026-10-14 14:17:49 +00:00
fix(ls): keep the PDEATHSIG registration alive across the calling thread's own exit
Tool calls and language server startup dispatch Popen() onto a short-lived thread (e.g. LanguageServerManager's per-server StartLSThread), which terminates moments after Popen() returns. PR_SET_PDEATHSIG ties its registration to that specific calling thread, not the process, so the language server received SIGTERM and died within milliseconds of a normal startup on Linux CI, independent of any real SIGKILL scenario. Route Popen() calls that set preexec_fn=set_pdeathsig_on_parent_exit through one dedicated, permanently-running daemon thread instead, so the registration stays tied to a thread that only ever dies alongside the whole process. Added a regression test that starts a language server on a short-lived thread and checks it survives past that thread's exit.
This commit is contained in:
3 files changed
+155
-1
No files matched your search
@@ -532,9 +532,14 @@ class StdioLanguageServer(LanguageServerInterface):
|
||||
kwargs["preexec_fn"] = subprocess_util.set_pdeathsig_on_parent_exit
|
||||
# the language server is launched with binary (bytes) pipes; the cast is needed because the
|
||||
# presence of platform-specific **kwargs prevents ty from selecting the bytes Popen overload
|
||||
# popen_preserving_pdeathsig (rather than a plain subprocess.Popen call) matters specifically
|
||||
# when use_pdeathsig is set: the actual fork() has to happen on a thread that outlives the
|
||||
# calling thread, which may itself be short-lived (e.g. LanguageServerManager's per-server
|
||||
# StartLSThread) -- see its docstring for why a plain Popen call here would defeat the fix
|
||||
# for #1490 in exactly that case.
|
||||
process = cast(
|
||||
"subprocess.Popen[bytes]",
|
||||
subprocess.Popen(
|
||||
subprocess_util.popen_preserving_pdeathsig(
|
||||
cmd,
|
||||
stdout=subprocess.PIPE,
|
||||
stdin=subprocess.PIPE,
|
||||
|
||||
@@ -1,7 +1,10 @@
|
||||
import logging
|
||||
import platform
|
||||
import queue
|
||||
import signal
|
||||
import subprocess
|
||||
import threading
|
||||
from collections.abc import Callable
|
||||
|
||||
import oslex
|
||||
import psutil
|
||||
@@ -32,11 +35,85 @@ def set_pdeathsig_on_parent_exit() -> None:
|
||||
program the shell forks as a child; see convert_shell_cmd's `exec` prefix, which makes the
|
||||
shell replace itself with the program instead of forking it, so the same PID (and therefore
|
||||
the same PDEATHSIG registration, which execve preserves) carries through.
|
||||
|
||||
Warning this function's caller (popen_preserving_pdeathsig below) exists to guard against: per
|
||||
`man 2 prctl`, PR_SET_PDEATHSIG's "parent" is specifically the OS THREAD that called fork() to
|
||||
create this process, not the parent process as a whole -- if that thread terminates while the
|
||||
rest of the parent process (e.g. its main thread) keeps running, the death signal fires anyway.
|
||||
"""
|
||||
if _libc is not None:
|
||||
_libc.prctl(_PR_SET_PDEATHSIG, signal.SIGTERM)
|
||||
|
||||
|
||||
class _PDeathSigSpawner:
|
||||
"""
|
||||
Runs subprocess.Popen() calls that register PR_SET_PDEATHSIG on one dedicated, permanently
|
||||
running daemon thread, so the "parent thread" the kernel ties the registration to is never a
|
||||
short-lived one.
|
||||
|
||||
Why this exists: StdioLanguageServer._start() (the sole caller of popen_preserving_pdeathsig)
|
||||
is very often invoked from a thread that is *intentionally* short-lived. For example,
|
||||
LanguageServerManager.from_languages spawns one StartLSThread per language server; each one
|
||||
calls language_server.start() (-> ._start(), where Popen() happens) and then simply returns,
|
||||
terminating moments after Popen() itself returns. Because PR_SET_PDEATHSIG's delivery is tied
|
||||
to that specific calling thread (see the warning on set_pdeathsig_on_parent_exit above), the
|
||||
language server would then receive the parent-death signal and exit almost immediately, even
|
||||
though Serena's process is still fully alive -- the exact CI-only regression this class fixes.
|
||||
Funneling the fork()/exec() itself through one thread that never voluntarily exits (it only
|
||||
ever terminates alongside the whole process, which is exactly the case PR_SET_PDEATHSIG is
|
||||
meant to detect) decouples "which caller thread happened to invoke _start()" from "does the
|
||||
language server survive."
|
||||
"""
|
||||
|
||||
def __init__(self) -> None:
|
||||
self._queue: queue.Queue[tuple[Callable[[], subprocess.Popen], "queue.Queue"]] = queue.Queue()
|
||||
self._thread = threading.Thread(target=self._run, name="solidlsp-pdeathsig-spawner", daemon=True)
|
||||
self._thread.start()
|
||||
|
||||
def _run(self) -> None:
|
||||
while True:
|
||||
func, result_queue = self._queue.get()
|
||||
try:
|
||||
result_queue.put((None, func()))
|
||||
except BaseException as e:
|
||||
result_queue.put((e, None))
|
||||
|
||||
def spawn(self, func: Callable[[], subprocess.Popen]) -> subprocess.Popen:
|
||||
result_queue: queue.Queue = queue.Queue(maxsize=1)
|
||||
self._queue.put((func, result_queue))
|
||||
error, process = result_queue.get()
|
||||
if error is not None:
|
||||
raise error
|
||||
return process
|
||||
|
||||
|
||||
class _SpawnerHolder:
|
||||
instance: "_PDeathSigSpawner | None" = None
|
||||
|
||||
|
||||
_pdeathsig_spawner_holder = _SpawnerHolder()
|
||||
_pdeathsig_spawner_lock = threading.Lock()
|
||||
|
||||
|
||||
def popen_preserving_pdeathsig(cmd: str | list[str], **kwargs) -> subprocess.Popen:
|
||||
"""
|
||||
subprocess.Popen wrapper for spawns that pass preexec_fn=set_pdeathsig_on_parent_exit.
|
||||
|
||||
On Linux, delegates the actual Popen() call to a single dedicated daemon thread (see
|
||||
_PDeathSigSpawner) instead of performing it on the calling thread directly, so that the
|
||||
PR_SET_PDEATHSIG registration set up by the preexec_fn remains tied to a thread that lives as
|
||||
long as the process does. On any other platform (or if no preexec_fn is passed), this is
|
||||
exactly equivalent to calling subprocess.Popen(cmd, **kwargs) directly.
|
||||
"""
|
||||
if platform.system() != "Linux" or kwargs.get("preexec_fn") is None:
|
||||
return subprocess.Popen(cmd, **kwargs)
|
||||
with _pdeathsig_spawner_lock:
|
||||
if _pdeathsig_spawner_holder.instance is None:
|
||||
_pdeathsig_spawner_holder.instance = _PDeathSigSpawner()
|
||||
spawner = _pdeathsig_spawner_holder.instance
|
||||
return spawner.spawn(lambda: subprocess.Popen(cmd, **kwargs))
|
||||
|
||||
|
||||
def subprocess_kwargs() -> dict:
|
||||
"""
|
||||
Returns a dictionary of keyword arguments for subprocess calls, adding platform-specific
|
||||
|
||||
@@ -62,6 +62,78 @@ def _find_marked_processes(marker: str) -> list[psutil.Process]:
|
||||
return found
|
||||
|
||||
|
||||
_THREAD_DRIVER_SRC = textwrap.dedent(
|
||||
"""
|
||||
import sys
|
||||
import threading
|
||||
import time
|
||||
from solidlsp.ls_config import LanguageServerId
|
||||
from solidlsp.ls_process import StdioLanguageServer
|
||||
from solidlsp.lsp_protocol_handler.server import ProcessLaunchInfo
|
||||
|
||||
marker = sys.argv[1]
|
||||
|
||||
# Mirrors LanguageServerManager.from_languages's StartLSThread: the real code path spawns one
|
||||
# Thread per language server, that thread calls language_server.start() (-> ._start(), where
|
||||
# the process is actually launched), and the thread then simply returns -- terminating almost
|
||||
# immediately after Popen() returns, not on the process's main thread.
|
||||
def start_language_server():
|
||||
ls = StdioLanguageServer(
|
||||
process_launch_info=ProcessLaunchInfo(
|
||||
cmd=[sys.executable, "-c", f"import time; time.sleep(300) # {marker}"],
|
||||
cwd="/tmp",
|
||||
),
|
||||
ls_id=LanguageServerId.PYTHON,
|
||||
determine_log_level=lambda _line: 0,
|
||||
)
|
||||
ls.start()
|
||||
|
||||
t = threading.Thread(target=start_language_server)
|
||||
t.start()
|
||||
t.join() # the calling thread is gone from here on; the driver process itself lives on
|
||||
print("READY", flush=True)
|
||||
time.sleep(600)
|
||||
"""
|
||||
)
|
||||
|
||||
|
||||
def test_language_server_process_survives_a_short_lived_calling_thread() -> None:
|
||||
"""
|
||||
PR_SET_PDEATHSIG (per prctl(2)) ties the registration to the specific calling *thread*, not
|
||||
the process: if that thread terminates while the rest of the process lives on, the kernel
|
||||
delivers the death signal right then, even though "Serena" (the process) never died. Starting
|
||||
a language server is dispatched through TaskExecutor onto exactly such a short-lived thread, so
|
||||
a naive preexec_fn registration kills the language server itself within milliseconds of a normal
|
||||
startup, independent of any real SIGKILL scenario -- this is the CI-only regression this test
|
||||
guards against.
|
||||
"""
|
||||
marker = f"pdeathsig-thread-test-{uuid.uuid4().hex}"
|
||||
driver = subprocess.Popen(
|
||||
[sys.executable, "-c", _THREAD_DRIVER_SRC, marker],
|
||||
stdout=subprocess.PIPE,
|
||||
stderr=subprocess.PIPE,
|
||||
text=True,
|
||||
)
|
||||
try:
|
||||
ready_line = driver.stdout.readline()
|
||||
assert ready_line.strip() == "READY", f"driver failed to start the language server: {driver.stderr.read()}"
|
||||
|
||||
time.sleep(2)
|
||||
# The driver's own argv also contains `marker` (it's passed as sys.argv[1]), so exclude
|
||||
# the driver's own pid or it would satisfy the assertion below on its own, regardless of
|
||||
# whether the actual language server process (its child) is still alive.
|
||||
survivors = [p for p in _find_marked_processes(marker) if p.pid != driver.pid]
|
||||
assert survivors, (
|
||||
"language server process died on its own even though the driver ('Serena') process "
|
||||
"is still alive -- the calling thread's exit, not a real parent death, killed it"
|
||||
)
|
||||
finally:
|
||||
for proc in _find_marked_processes(marker):
|
||||
proc.kill()
|
||||
if driver.poll() is None:
|
||||
driver.kill()
|
||||
|
||||
|
||||
def test_language_server_process_dies_with_a_sigkilled_serena() -> None:
|
||||
marker = f"pdeathsig-test-{uuid.uuid4().hex}"
|
||||
driver = subprocess.Popen(
|
||||
|
||||
Reference in new issue
Block a user