perf(gateway): read a PID's command line with psutil before forking ps
`_read_process_cmdline` tried `/proc`, then `ps`, then psutil. macOS and BSD have no `/proc`, so every call there forked `ps -p <pid> -o command=` while psutil — a pinned core dependency, which pyproject calls "the canonical answer" for PID questions — sat unused behind it, returning the same string in-process. That call is the gateway-identity check guarding against PID reuse, reached for every profile with a live gateway, and `list_profiles()` runs it per profile. `list_profiles` is the shared body of `GET /api/profiles` and `profiles.list`, which the Bots roster polls every 5s per connection, so the forks repeat forever on an idle machine. Measured on macOS: one call 4.17ms via `ps` against 0.022ms via psutil. End to end against a home whose profiles each hold a live gateway lock, A/B in one process state, two rounds: 4 gateways 23.9ms -> 4.0ms, 8 gateways 49.0ms -> 9.3ms. `ps` stays as the fallback rather than being replaced: on macOS psutil raises AccessDenied for a process owned by another user, which `ps` still reports — real when a dashboard probes root-owned LaunchDaemon gateways. That path is unchanged and simply pays a cheap failed lookup first. For a readable process both sources return byte-identical strings, so the command-line matchers see no difference, and Linux still answers from /proc without reaching either. Regressions: psutil answers without any fork (a raising `subprocess.run` proves it); the `ps` fallback still answers when psutil refuses, as it does across users; a dead PID reports nothing from either; and both sources agree on a live gateway-shaped process. Restoring the old order fails the first. Fixes #117270
This commit is contained in:
committed by
Teknium
parent
7a23b00101
commit
5d775ff8ee
@@ -450,11 +450,22 @@ def get_process_start_time(pid: int) -> Optional[int]:
|
||||
|
||||
|
||||
def _read_process_cmdline(pid: int) -> Optional[str]:
|
||||
"""Process command line as one string: /proc, then ``ps``, then psutil (Windows)."""
|
||||
"""Process command line as one string: /proc, then psutil, then ``ps``.
|
||||
|
||||
Order is by cost, and this runs per live gateway on every roster/status poll. ``psutil`` reads
|
||||
the process table in-process (a ``sysctl`` on macOS) where ``ps`` costs a fork+exec — measured
|
||||
0.02ms against 4.2ms on macOS for the same string. It cannot always answer: on macOS it raises
|
||||
``AccessDenied`` for a process owned by another user, which ``ps`` still reports, so ``ps``
|
||||
stays as the fallback rather than being replaced."""
|
||||
with contextlib.suppress(OSError):
|
||||
raw = Path(f"/proc/{pid}/cmdline").read_bytes()
|
||||
if raw:
|
||||
return raw.replace(b"\x00", b" ").decode("utf-8", errors="ignore").strip()
|
||||
with contextlib.suppress(Exception):
|
||||
import psutil # type: ignore
|
||||
cmdline_parts = psutil.Process(pid).cmdline()
|
||||
if cmdline_parts:
|
||||
return " ".join(cmdline_parts)
|
||||
if not _IS_WINDOWS:
|
||||
with contextlib.suppress(OSError, subprocess.TimeoutExpired):
|
||||
result = subprocess.run(
|
||||
@@ -463,11 +474,6 @@ def _read_process_cmdline(pid: int) -> Optional[str]:
|
||||
)
|
||||
if result.returncode == 0 and result.stdout.strip():
|
||||
return result.stdout.strip()
|
||||
with contextlib.suppress(Exception):
|
||||
import psutil # type: ignore
|
||||
cmdline_parts = psutil.Process(pid).cmdline()
|
||||
if cmdline_parts:
|
||||
return " ".join(cmdline_parts)
|
||||
return None
|
||||
|
||||
|
||||
|
||||
103
tests/gateway/test_process_cmdline_source.py
Normal file
103
tests/gateway/test_process_cmdline_source.py
Normal file
@@ -0,0 +1,103 @@
|
||||
"""``_read_process_cmdline`` asks the cheapest source that can answer, and still answers.
|
||||
|
||||
The gateway-identity check reads a live PID's command line, and `list_profiles` runs it per profile
|
||||
with a live gateway — on every 5s roster poll, per connection. On macOS there is no ``/proc``, so
|
||||
this used to fork ``ps`` every time (measured 4.2ms) when psutil answers the same string in 0.02ms.
|
||||
psutil cannot always answer (macOS raises ``AccessDenied`` across users), so ``ps`` must remain the
|
||||
fallback, not be replaced.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import subprocess
|
||||
import sys
|
||||
import time
|
||||
|
||||
import pytest
|
||||
|
||||
from gateway import status
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def no_proc(monkeypatch):
|
||||
"""Force the non-Linux shape: /proc unreadable, so the source choice is psutil vs ps."""
|
||||
real_read_bytes = status.Path.read_bytes
|
||||
|
||||
def _read_bytes(self, *a, **kw):
|
||||
if str(self).startswith("/proc/"):
|
||||
raise OSError("no /proc")
|
||||
return real_read_bytes(self, *a, **kw)
|
||||
|
||||
monkeypatch.setattr(status.Path, "read_bytes", _read_bytes)
|
||||
|
||||
|
||||
def _no_fork(monkeypatch):
|
||||
"""Fail loudly if anything forks a subprocess."""
|
||||
def _boom(*a, **kw):
|
||||
raise AssertionError(f"forked a subprocess: {a[0] if a else kw}")
|
||||
|
||||
monkeypatch.setattr(status.subprocess, "run", _boom)
|
||||
|
||||
|
||||
def test_psutil_answers_without_forking_ps(no_proc, monkeypatch):
|
||||
_no_fork(monkeypatch)
|
||||
|
||||
cmdline = status._read_process_cmdline(os.getpid())
|
||||
|
||||
assert cmdline and sys.executable.split("/")[-1] in cmdline
|
||||
|
||||
|
||||
def test_ps_still_answers_when_psutil_cannot(no_proc, monkeypatch):
|
||||
"""macOS raises AccessDenied across users; ps still reports those, so it stays the fallback."""
|
||||
import psutil
|
||||
|
||||
def _denied(_pid):
|
||||
raise psutil.AccessDenied(_pid)
|
||||
|
||||
monkeypatch.setattr(psutil, "Process", _denied)
|
||||
forked: list = []
|
||||
real_run = status.subprocess.run
|
||||
|
||||
def _watching(cmd, *a, **kw):
|
||||
forked.append(cmd)
|
||||
return real_run(cmd, *a, **kw)
|
||||
|
||||
monkeypatch.setattr(status.subprocess, "run", _watching)
|
||||
|
||||
cmdline = status._read_process_cmdline(os.getpid())
|
||||
|
||||
assert cmdline, "the ps fallback must still answer when psutil refuses"
|
||||
assert forked and forked[0][:2] == ["ps", "-p"]
|
||||
|
||||
|
||||
def test_a_dead_pid_reports_nothing_from_either_source(no_proc):
|
||||
dead = subprocess.Popen([sys.executable, "-c", "pass"])
|
||||
dead.wait()
|
||||
|
||||
assert status._read_process_cmdline(dead.pid) in (None, "")
|
||||
|
||||
|
||||
def test_both_sources_agree_on_a_live_process(no_proc, monkeypatch):
|
||||
"""The consumer matches on this string, so the two sources must not disagree."""
|
||||
import psutil
|
||||
|
||||
proc = subprocess.Popen([sys.executable, "-c", "import time; time.sleep(30)", "gateway", "run"])
|
||||
try:
|
||||
for _ in range(50):
|
||||
if psutil.pid_exists(proc.pid):
|
||||
break
|
||||
time.sleep(0.05)
|
||||
via_psutil = status._read_process_cmdline(proc.pid)
|
||||
|
||||
denied = psutil.AccessDenied
|
||||
|
||||
def _raise(_pid):
|
||||
raise denied(_pid)
|
||||
|
||||
monkeypatch.setattr(psutil, "Process", _raise)
|
||||
via_ps = status._read_process_cmdline(proc.pid)
|
||||
finally:
|
||||
proc.kill()
|
||||
proc.wait()
|
||||
|
||||
assert via_psutil == via_ps
|
||||
Reference in New Issue
Block a user