Fix busy loop when a peer disconnects in the middle of a message header - #2073
Conversation
JsonIOStream._read_line() checks for EOF by testing the accumulated line rather than the bytes it just read. Once any part of a header line has arrived, that accumulator is never empty again, so the `if not line` check can only ever fire on the first read. A stream that ends mid-line keeps returning b"" from readline(), the line never grows and never ends with CRLF, and the loop spins with no sleep and no blocking call. Sockets here are always blocking (from_socket() does settimeout(None)), so b"" from readline() means EOF and nothing else. Test the chunk instead, and treat a truncated header the same way the body loop below already treats a truncated body: no more messages. The clean disconnect path is unchanged, since the first read then returns b"" with the accumulator still empty. This is reachable on any adapter started with --listen, from a peer that writes a few bytes without a CRLF and closes, and from a client or debuggee that dies while a header is partially flushed. The message loop thread then pins a core instead of shutting the session down.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
🔒 Automated review in progress — Bill Schnurr (@bschnurr) is auto-reviewing this PR. |
|
Result: Verification detailsVerification: Isolated verification observed failures that were not classified as caused by this PR: Offline editable dependency bootstrap. The relevant tests could not be fully run in the isolated environment; this review is not fully verified. Summary: The new truncated-header regression test passed, and the complete messaging, JSON, and socket test files passed with 25 total test executions. The initial test invocation could not import `debugpy`, and editable installation failed because the sandbox lacked `setuptools.build_meta`; running directly from `src` provided full targeted coverage. Verification is partial because the standard installation path could not be established, though no test assertion failed. Test runs: 4 passed, 1 failed, 1 not run
|
Bill Schnurr (bschnurr)
left a comment
There was a problem hiding this comment.
Approved via Review Center.
Rich Chiodo (rchiodo)
left a comment
There was a problem hiding this comment.
Approved via Review Center.
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Thanks for the PR. |
Fixes #2072
JsonIOStream._read_line()decides it has hit EOF by testingline, the accumulator it is building up, rather than the bytes it just read. Once any part of a header line has arrived,lineis never empty again, soif not linecan only ever fire on the very first read. When a stream ends part way through a header,readline()returnsb""from then on,linenever grows and never ends with CRLF, and the loop spins. No sleep, nothing to block on, no bound.readline()returningb""here means EOF and only EOF.from_socket()doessock.settimeout(None), and the only othersettimeoutcalls on a DAP stream outside_vendoredalso passNone. The one real timeout, insockets.py, is on the listening socket and not on a stream. So the fix is to test the chunk, and to treat a truncated header the way the body loop a few lines further down already treats a truncated body: there are no more messages. The clean disconnect path is untouched, since the firstreadline()then returnsb""while the accumulator is still empty.What changed:
_read_line()reads intochunk, raisesNoMoreMessageswhenchunkis empty, then appends it toline.TestJsonIOStream.test_read_truncated_headerfeeds a reader that yieldsb"Content-Length: 24"and then EOF forever, and assertsNoMoreMessagesplus exactly one read past EOF.ReaderSpinningafter ten reads past EOF instead of hanging, so on the old code the test fails in under a second with a message that names the problem, rather than sitting there until the suite timeout fires.io.RawIOBase, so it satisfies thereaderannotation onJsonIOStream.__init__, and itsreadlinetakessize: int | None = -1to match theIOBasesignature.Testing, on macOS 26.6.2 with CPython 3.10.6, each run with
-o addopts=to keep it off the 8 worker default:pytest tests/debugpy/common/test_messaging.py -k "not fuzz": 13 passed, 1 deselected in 2.32s.pytest tests/debugpy/common/test_messaging.py -k "fuzz": 1 passed, 13 deselected in 0.42s.pytest tests/debugpy/common/test_json.py tests/debugpy/common/test_socket.py: 10 passed in 1.76s.src/debugpy/common/messaging.pyput back to e220805 and the new test kept: 1 failed, 12 passed, 1 deselected in 2.23s. The failure isReaderSpinning: readline() was called 11 times at EOF, raised out ofmessaging.py:182.socket.socketpair(). Before, the truncated case reportedfinished=False CPU=2.93s. After,finished=True CPU=0.00s. The case where the peer closes without sending anything readsfinished=True CPU=0.00sboth before and after.python -m ruff check ., which is what the Lint stage runs: all checks passed, on ruff 0.16.8.standardmode, ontests/debugpy/common/test_messaging.py: 5 errors, the same 5 that are on it at e220805, allreportOptionalMemberAccessin tests this PR does not touch. The new helper adds none.python -m flake8 src/debugpy/common/messaging.py tests/debugpy/common/test_messaging.py: the same 5 findings as on e220805, all of them in code this PR does not touch. Nothing new. The test file is clean.I left the same unbounded shape in
write_json()as it is. I could not build a writer that reaches it, so there would be nothing behind the change.