Skip to content

Commit 04bd449

Browse files
rchiodoCopilot
andcommitted
Fail fast on None invariants instead of degraded fallbacks
Address review feedback on the typing pass: replace silently-degraded None fallbacks with fail-fast asserts, and make ThreadSafeSingleton.assert_locked side-effect-free. - singleton.py: assert_locked now checks lock._is_owned() instead of acquire()/release(), avoiding RLock recursion-count mutation and an unbalanced release() under python -O. - servers.py: assert pydevdSystemInfo result is non-exception (pid/ppid always assigned before use); assert listener in inject() rather than connecting to an empty address. - clients.py: assert servers.listener in attach_request and the client listener in notify_of_subprocess instead of substituting ("", 0)/None. - debuggee.py: assert process in wait_for_exit rather than reporting a bogus clean exit (code 0). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 1478a48 commit 04bd449

4 files changed

Lines changed: 31 additions & 14 deletions

File tree

src/debugpy/adapter/clients.py

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -490,11 +490,10 @@ def attach_request(self, request):
490490
else:
491491
if not servers.is_serving():
492492
servers.serve(localhost)
493-
host, port = (
494-
sockets.get_address(servers.listener)
495-
if servers.listener is not None
496-
else ("", 0)
497-
)
493+
# servers.serve() above guarantees a listener; fail fast if it's missing
494+
# rather than handing the debuggee an empty ("", 0) address it can't use.
495+
assert servers.listener is not None
496+
host, port = sockets.get_address(servers.listener)
498497

499498
# There are four distinct possibilities here.
500499
#
@@ -772,7 +771,11 @@ def notify_of_subprocess(self, conn):
772771
localhost = sockets.get_default_localhost()
773772
body["connect"]["host"] = host or localhost
774773
if "port" not in body["connect"]:
775-
if port is None and listener is not None:
774+
if port is None:
775+
# A subprocess is only reported once the client is connected, so the
776+
# client listener must be serving; fail fast rather than sending the
777+
# child a null port it can't connect to.
778+
assert listener is not None
776779
_, port = sockets.get_address(listener)
777780
body["connect"]["port"] = port
778781

src/debugpy/adapter/servers.py

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -80,10 +80,13 @@ def __init__(self, sock: socket.socket):
8080
try:
8181
self.authenticate()
8282
info = self.channel.request("pydevdSystemInfo")
83-
if not isinstance(info, Exception):
84-
process_info: Callable[..., int] = cast(Callable[..., int], info("process", json.object()))
85-
self.pid = process_info("pid", int)
86-
self.ppid = process_info("ppid", int, optional=True)
83+
# channel.request() either returns a non-exception result or raises, so
84+
# assert (rather than silently skipping) to fail fast and to narrow the
85+
# type, keeping pid/ppid unconditionally assigned before they're read.
86+
assert not isinstance(info, Exception)
87+
process_info: Callable[..., int] = cast(Callable[..., int], info("process", json.object()))
88+
self.pid = process_info("pid", int)
89+
self.ppid = process_info("ppid", int, optional=True)
8790
if self.ppid == ():
8891
self.ppid = None
8992
self.channel.name = stream.name = str(self)
@@ -484,7 +487,11 @@ def dont_wait_for_first_connection():
484487

485488

486489
def inject(pid, debugpy_args, on_output):
487-
host, port = sockets.get_address(listener) if listener is not None else ("", 0)
490+
# inject() is only reached for attach-by-PID, where the server listener must be
491+
# serving so the injected debug server can connect back. Fail fast if it isn't,
492+
# rather than spawning an injector that connects to ":0" and never attaches.
493+
assert listener is not None
494+
host, port = sockets.get_address(listener)
488495

489496
cmdline = [
490497
sys.executable,

src/debugpy/common/singleton.py

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -144,12 +144,15 @@ def __init__(self, *args, **kwargs):
144144
def assert_locked(self):
145145
lock = type(self)._lock
146146
assert lock is not None
147-
assert lock.acquire(blocking=False), (
147+
# Side-effect-free check: verify the current thread already owns the lock,
148+
# rather than acquire()/release() (which mutates the RLock recursion count
149+
# and, under `python -O` with the assert stripped, would leak a release()).
150+
# _is_owned() is a private but stable RLock helper (used by threading itself).
151+
assert lock._is_owned(), ( # pyright: ignore[reportAttributeAccessIssue]
148152
"ThreadSafeSingleton accessed without locking. Either use with-statement, "
149153
"or if it is a method or property, mark it as @threadsafe_method or with "
150154
"@autolocked_method, as appropriate."
151155
)
152-
lock.release()
153156

154157
def __getattribute__(self, name):
155158
value = object.__getattribute__(self, name)

src/debugpy/launcher/debuggee.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,11 @@ def kill():
198198

199199
def wait_for_exit():
200200
try:
201-
code = process.wait() if process is not None else 0
201+
# wait_for_exit() only runs after the debuggee was spawned successfully, so
202+
# process is always set here; assert to fail fast rather than reporting a
203+
# bogus clean exit (code 0) if that invariant is ever broken.
204+
assert process is not None
205+
code = process.wait()
202206
if sys.platform != "win32" and code < 0:
203207
# On POSIX, if the process was terminated by a signal, Popen will use
204208
# a negative returncode to indicate that - but the actual exit code of

0 commit comments

Comments
 (0)