Repository navigation
Fail self.deferred in connectionLost instead of hanging forever - #543
Merged
Merged
Conversation
sibson
commented
Sep 22, 2026
captureScreen/captureRegion, expectScreen/expectRegion, refreshScreen and stableScreen/stableRegion each park one pending operation in self.deferred (or, for the stable-watch commands, ride on it indirectly). None of them learned about a dropped connection: the Deferred a caller was waiting on simply never fired, so a disconnect mid-capture hung the caller rather than failing it. connectionLost now errbacks self.deferred with a new ConnectionLostError when one is pending -- ProtocolError and DesktopResizeError already name a specific cause, neither of which is "the socket went away", so a new exception says that plainly. _StableWatch is the one case that needed more than an errback on self.deferred: refreshScreen's Deferred that _request chains off is one round of the watch, not the Deferred that stableScreen/stableRegion handed to their caller (_StableWatch.result, only ever fired from _settle()). Failing self.deferred alone would have hit "Unhandled error in Deferred" and left the caller hanging exactly as before, just one layer down. _request now adds an errback alongside its callback that fails self.result directly, cancelling the pending settle timer so a stale timer cannot later call back an already-errored Deferred. For refreshScreen/_capture/expectScreen, no such wiring was needed: those hand the caller self.deferred itself (or a Deferred Twisted chains onto it across rounds), so errbacking self.deferred already reaches the caller through Twisted's ordinary Deferred-returning- Deferred chaining. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V9XU3qUSZYysSFCZNaRpQc
sibson
force-pushed
the
fix/deferred-fails-on-disconnect
branch
from
September 22, 2026 23:49
c8bcfa7 to
0ee30c2
Compare
Owner
Author
|
Cause, measured since on draft #546: TightVNC serves an all-black frame (every tile a Tight fill of I can't re-run the failed job (403), so this PR stays watched. Generated by Claude Code |
sibson
commented
Sep 23, 2026
The bool flipped to True in exactly the two places the caller's Deferred fired (_settle's callback, _disconnected's errback), so it restated Deferred.called under a second name. Keep one: the Deferred is now `settled`, and the re-arm guards test `settled.called`. "Settled" in the promise sense covers both outcomes, which is what the disconnect path added. `result` was too generic a name for it, per review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
capture,expect,refreshScreenandstableall wait onself.deferred, and none of them learned about a dropped connection, so a disconnect mid-call hung the caller.connectionLostnow errbacks a pendingself.deferredwith a newConnectionLostError._StableWatchhands its caller a separate Deferred, fired by a timer rather than by an update, so_requestalso errbacks that one and cancels the timer. Review renamed itresult→settledand dropped the bool that duplicatedsettled.called.resizeScreenkeeps its own_PendingResizeDeferred: it completes on the server's ExtendedDesktopSize reply, not oncommitUpdate, so it can't share the slot.connectionLostchecks both, which is where this conflicted with #497.🤖 Generated with Claude Code