fix(protocol): retransmit and fail fast on the write path - #1
Closed
mbillow wants to merge 1 commit into
Closed
Conversation
post() sent its datagram exactly once and waited on a bare ev.wait(), while get() retransmits every block through _exchange_block. One lost datagram was therefore an unrecoverable write and a silent no-op for a read -- the asymmetry behind three unrelated resources on one AC all failing with SessionTimeoutError (LocalThings#384). - Extract _wait_live() from _wait_for_block() and wait through it in post(). A reader thread dying mid-write now raises SessionClosedError within a liveness poll instead of burning the caller's whole timeout and reporting a dead session as a device timeout. - Retransmit the CON up to write_max_attempts times inside the caller's deadline, reusing the same Message ID. The MID reuse is what makes retransmitting a non-idempotent POST safe: a server implementing RFC 7252 4.5 answers the duplicate from its dedupe cache rather than re-running the write, which a caller-side retry can never offer since it mints a fresh MID. Defaults to 1 attempt -- byte-for-byte today's behaviour -- because a device already dropping under load turns one lost write into several, and RT-OCF's dedupe is unverified. Enable it once pacing has been shown insufficient on real hardware. - Pace the dereg sweep in refresh_observes(). Unlike the teardown dereg in close(), which wants out quickly, this one runs against a session that has to keep working afterwards, and an unpaced OBSERVE burst is what wedges an appliance until something forces a new session (LocalThings#396). Pacing of the request paths themselves is deliberately left alone: subscribe(), post() and block zero are QuiteYellow#51, and a second layer at the call sites would only have to be unwound when it lands.
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.
Picks up the half of mbillow/localthings#384 that QuiteYellow left open after QuiteYellow#51 claimed the pacing half.
The asymmetry
post()sends its datagram exactly once:get()goes through_exchange_block, which retransmits each block up to_BLOCK_MAX_ATTEMPTSat_BLOCK_ACK_TIMEOUTper attempt. So one lost datagram — request or ACK — is an unrecoverable write, while a read absorbs the identical loss silently. That matches the report in #384: reads keep working, three unrelated resources (/power/vs/0,/temperature/desired/0,/wind/direction/vs/0) intermittently don't.The bare
ev.wait()is a second, separate gap: it skips the_wait_for_blockliveness slicingget()uses, so a reader thread dying mid-write burns the full 8 s and reports a device timeout for what is actually a dead session.What this does
Liveness (active).
_wait_for_blockbecomes_wait_live, andpost()waits through it. A reader death mid-write now raisesSessionClosedErrorwithin one liveness poll instead of at the end of the caller's timeout.Retransmission (off by default).
post()retransmits the CON up towrite_max_attemptstimes inside the caller's deadline, with §4.2 backoff, pacing each retransmit, and the final attempt taking whatever budget is left.The datagram is built once and resent verbatim. That MID reuse is the load-bearing part: a server implementing RFC 7252 §4.5 recognises the duplicate and answers from its dedupe cache instead of re-running the write. A caller-side retry can't offer that —
post()mints a fresh MID and token per call, so a retry from above is a genuinely new request the device has no way to dedupe.It defaults to 1 attempt — byte-for-byte today's behaviour — per @QuiteYellow's ordering caution on #384: retransmitting into a device that's already dropping under load turns one lost write into several, and §4.5 dedupe is unverified on RT-OCF, which doesn't reliably emit RST either. With QuiteYellow#51 landed we can see whether writes are still being lost before turning this on, and the flag is then a one-line change rather than a new feature.
refresh_observes()dereg sweep (active). Paced. Unlike the teardown dereg inclose(), which wants out quickly, this one runs against a session that has to keep working afterwards, and an unpaced OBSERVE burst is what wedges an appliance (mbillow/localthings#396).What this deliberately does not touch
Pacing of
subscribe(),post(), and block zero — that's QuiteYellow#51, and putting a second layer at the call sites would only have to be unwound when it lands. Thetime.sleep(0.05)in therefresh_observessubscribe sweep is left alone for the same reason:subscribe()is where that belongs.Tests
tests/test_dtls_session_post_retry.py, on the existing_FakeConn/_FakeSockharness:post()raisesSessionClosedErrorfast, notSessionTimeoutErrorrefresh_observespaces its dereg sweepFull suite: 275 passed.
Generated by Claude Code