fix(dtls): make reader-thread death visible and fail fast - #38
Merged
Conversation
The reader loop exited silently on any socket error, leaving conn/sock set so the session still looked open. Every later get()/post()/ping() then waited out its full request timeout on a session nobody was reading, raising SessionTimeoutError on repeat, forever. This started biting in v0.1.3 (d677c72), which moved to connected UDP sockets: a connected socket surfaces ICMP errors on recv, so one ECONNREFUSED from a rebooting appliance now killed the reader. - Advisory ICMP errnos (ECONNREFUSED/EHOSTUNREACH/...) no longer kill the reader; the next datagram usually works. - Real reader exits log at WARNING; close()-driven exits stay quiet. - A _reader_running Event lets get/post/ping/subscribe/refresh_observes fail fast via _check_live() with SessionClosedError instead of waiting out a timeout. Callers that never start a reader are unaffected. Refs #37
Owner
Author
Contributor
|
It’ll get lumped into the next LocalThings release. |
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
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.
Problem
_reader_loopexited silently on any socket error with a barereturn. Nothing was logged, andself.conn/self.sockstayed set, so the session still looked open to every caller. Each laterget()/post()/ping()passed itsconn is Noneguard, sent its CON, and waited on anEventno thread would ever set — three attempts at four seconds each, thenSessionTimeoutError, on every call, forever.This started biting in v0.1.3.
d677c72moved the session onto connected UDP sockets, and a connected UDP socket surfaces ICMP errors on the nextrecv. A singleECONNREFUSEDfrom an appliance whose DTLS port is momentarily closed (rebooting, router blip) now kills the reader, where an unconnected socket had simply timed out and looped.Refs #37 — this is the invisible-failure half of what charettepa is seeing. It does not claim to fix a device-side wedge; it makes the failure visible and fast instead of silent and slow.
Changes
ECONNREFUSED,EHOSTUNREACH,ENETUNREACH,EHOSTDOWN,ENETDOWNlog at DEBUG andcontinue— UDP delivery was never guaranteed and the next datagram usually works.close()-driven exit (_stopalready set) still returns quietly, so normal teardown stays silent._reader_runningEvent is set instart_reader()and cleared in the loop'sfinally.get/post/ping/subscribe/refresh_observesnow go through_check_live(), which raisesSessionClosedErrorimmediately when the reader is gone instead of waiting out the request timeout. Callers that never start a reader (config-flow style) are unaffected — the reader check only applies once_reader_threadis set._send_dgramkeeps its ownconn is Nonecheck unchanged:_dispatch_coapcalls it to auto-ACK device CONs, so it runs on the reader thread and must not raise during teardown.Behaviour change for downstream (mbillow/localthings)
A dead reader now raises
SessionClosedError, notSessionTimeoutError.SessionClosedErrorsubclassesSessionError/ConnectionError, notTimeoutError, so localthings'_poll_once→_defer_reconnect_forthree-timeout tolerance is bypassed and it reconnects on the first occurrence rather than after ~2 minutes of a device sitting unavailable. That is the intended outcome, but it changes reconnect timing and downstream should hear it from us.Tests
New
tests/test_dtls_session_reader_death.pydrives_reader_loopwith a scripted fake socket/conn:ECONNREFUSEDsurvives and the following datagram is still dispatched (one DEBUG line, no WARNING);EBADFexits with exactly one WARNING and clears_reader_running;get()after reader death raisesSessionClosedErrorin <1s (asserts elapsed time so a regression that reintroduces the wait is caught);close()teardown logs no WARNING;_check_live()with no reader started matches the oldconn is Noneguard.Full tracked suite: 168 passed.
Not settled here
Whether #37's fridge is wedged by the fixed source port (v0.1.1) or by something the connected socket introduced (v0.1.3). That still needs charettepa's version-pin bisect — but once this ships, the logs will say which of the two the device is actually doing.