feat(protocol): add shared MID exchange registry - #57
Conversation
There was a problem hiding this comment.
Approving and merging this.
It matches what #56 asked for in step 1. One structure serving both paths, allocated and indexed under the same lock before the send, and drained from close() and the reader finally through a single function. Leaving ping and Observe unregistered is the right call, since ping() never waits on a reply.
Two details I checked closely and both are right. ev.clear() sits inside the same locked block as the container read, so a response arriving between the check and the wait cannot be dropped. And publishing conn = None before the drain, with the second _check_live() after registration in post() and _exchange_block, closes the register-after-drain race properly.
Moving acknowledged onto the exchange also settles #36 review point 2, so that one is off your list before the rebase.
Three things I am taking on myself in a follow-up:
_exchange_blockallocates a fresh MID per attempt, so an unacknowledged retry still goes out as a new request. @mbillow flagged this in the body of #54 and it predates your work. This PR hands same-MID retransmission to #54, which covers the write path, leaving the read half unassigned.- A matching RST, a token already in flight, and MID exhaustion all surface as a bare
SessionError(). #23 added the typed hierarchy for exactly this. _next_available_mid_lockedwalks all 65,536 MIDs before giving up.
I will land those on top before calling the rebases. Thanks for turning this around so quickly.
QuiteYellow#57 landed the shared MID registry. Three follow-ups on top of it. _exchange_block registered a new exchange per attempt, so an unacknowledged retry went out under a fresh MID. RFC 7252 §4.2 defines a retransmission as the same message, and a new MID instead presents each retry to the appliance as a separate request it may answer twice. The registration and the datagram now happen once, outside the attempt loop, and every attempt sends the identical bytes. mbillow named this on QuiteYellow#56; it predates QuiteYellow#57. Two allocation failures also stopped being indistinguishable from any other session failure. A CoAP RST now raises SessionResetError, since it refuses one exchange while the transport stays up, and both "no free Message ID" and "this token is already in flight" raise SessionIdentifierError. _next_available_mid_locked probed all 65,536 identifiers before giving up. Candidates are consecutive and therefore distinct, so by the pigeonhole principle one more candidate than there are live exchanges always suffices, which is normally a single probe.
post() sent its datagram exactly once and then waited on a bare ev.wait(timeout), while get() retransmits every block through _exchange_block. One lost datagram — request or ACK — was therefore an unrecoverable write, while a read absorbed the identical loss silently. That is the report in LocalThings#384: reads keep working, three unrelated resources intermittently do not. Liveness. The bare wait also skipped the slicing _exchange_block uses, so a reader thread dying mid-write burned the caller's whole 8s and reported a device timeout for what was actually a dead session. _wait_for_block is no longer block-specific — it becomes _wait_live, and post() waits through it, so a reader death surfaces as SessionClosedError within one liveness poll. Retransmission, off by default. post() resends the CON up to write_max_attempts times inside the caller's deadline, with §4.2 backoff, pacing every attempt, and the last attempt taking whatever budget is left. The datagram is built once and resent verbatim; reusing the MID is the load-bearing part, because a server implementing §4.5 can then recognise the duplicate and answer from its dedupe cache instead of re-running the write. A caller-side retry cannot 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: on that path the wire behaviour is unchanged, one datagram sent in the same order as before, per the ordering caution on #384. Retransmitting into a device already dropping under load turns one lost write into several, and §4.5 dedupe is unverified on RT-OCF, which does not reliably emit RST either. With pacing (QuiteYellow#51) landed we can see whether writes are still lost before turning this on, and the flag is then a one-line change. Two details that are not carried over unchanged from the single-send version, both covered by tests: * timeout now bounds the whole call rather than the wait after the send. Attempts share one budget, so it has to be armed before the first pace — and a caller that asked for 8s should not wait 8s plus however long the rate limiter withheld the request. * a retransmission that fails to send is best-effort. A connected UDP socket reports the ICMP error queued by an earlier send on the next one, and the reader already treats those errnos as advisory; failing the exchange there would make retransmitting less robust than leaving it off. Attempt 0 still raises, since it is the caller's only datagram. Rebased onto the shared MID registry: the empty-ACK and RST matching this originally carried is QuiteYellow#57's now, and QuiteYellow#58 gave the read path the same one-datagram-per-exchange shape, so what remains here is the write attempt loop and the frames that must stop it. Attempt 0 keeps QuiteYellow#51's pace-then-check-then-send ordering exactly; the liveness recheck is skipped only once something has answered, because an answer that beat a dying reader is a write the device confirmed and must not be discarded.
post() sent its datagram exactly once and then waited on a bare ev.wait(timeout), while get() retransmits every block through _exchange_block. One lost datagram — request or ACK — was therefore an unrecoverable write, while a read absorbed the identical loss silently. That is the report in LocalThings#384: reads keep working, three unrelated resources intermittently do not. Liveness. The bare wait also skipped the slicing _exchange_block uses, so a reader thread dying mid-write burned the caller's whole 8s and reported a device timeout for what was actually a dead session. _wait_for_block is no longer block-specific — it becomes _wait_live, and post() waits through it, so a reader death surfaces as SessionClosedError within one liveness poll. Retransmission, off by default. post() resends the CON up to write_max_attempts times inside the caller's deadline, with §4.2 backoff, pacing every attempt, and the last attempt taking whatever budget is left. The datagram is built once and resent verbatim; reusing the MID is the load-bearing part, because a server implementing §4.5 can then recognise the duplicate and answer from its dedupe cache instead of re-running the write. A caller-side retry cannot 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: on that path the wire behaviour is unchanged, one datagram sent in the same order as before, per the ordering caution on #384. Retransmitting into a device already dropping under load turns one lost write into several, and §4.5 dedupe is unverified on RT-OCF, which does not reliably emit RST either. With pacing (QuiteYellow#51) landed we can see whether writes are still lost before turning this on, and the flag is then a one-line change. Two details that are not carried over unchanged from the single-send version, both covered by tests: * timeout now bounds the whole call rather than the wait after the send. Attempts share one budget, so it has to be armed before the first pace — and a caller that asked for 8s should not wait 8s plus however long the rate limiter withheld the request. * a retransmission that fails to send is best-effort. A connected UDP socket reports the ICMP error queued by an earlier send on the next one, and the reader already treats those errnos as advisory; failing the exchange there would make retransmitting less robust than leaving it off. Attempt 0 still raises, since it is the caller's only datagram. Rebased onto the shared MID registry: the empty-ACK and RST matching this originally carried is QuiteYellow#57's now, and QuiteYellow#58 gave the read path the same one-datagram-per-exchange shape, so what remains here is the write attempt loop and the frames that must stop it. Attempt 0 keeps QuiteYellow#51's pace-then-check-then-send ordering exactly; the liveness recheck is skipped only once something has answered, because an answer that beat a dying reader is a write the device confirmed and must not be discarded.
post() sent its datagram exactly once and then waited on a bare ev.wait(timeout), while get() retransmits every block through _exchange_block. One lost datagram — request or ACK — was therefore an unrecoverable write, while a read absorbed the identical loss silently. That is the report in LocalThings#384: reads keep working, three unrelated resources intermittently do not. Liveness. The bare wait also skipped the slicing _exchange_block uses, so a reader thread dying mid-write burned the caller's whole 8s and reported a device timeout for what was actually a dead session. _wait_for_block is no longer block-specific — it becomes _wait_live, and post() waits through it, so a reader death surfaces as SessionClosedError within one liveness poll. Retransmission, off by default. post() resends the CON up to write_max_attempts times inside the caller's deadline, with §4.2 backoff, pacing every attempt, and the last attempt taking whatever budget is left. The datagram is built once and resent verbatim; reusing the MID is the load-bearing part, because a server implementing §4.5 can then recognise the duplicate and answer from its dedupe cache instead of re-running the write. A caller-side retry cannot 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: on that path the wire behaviour is unchanged, one datagram sent in the same order as before, per the ordering caution on #384. Retransmitting into a device already dropping under load turns one lost write into several, and §4.5 dedupe is unverified on RT-OCF, which does not reliably emit RST either. With pacing (QuiteYellow#51) landed we can see whether writes are still lost before turning this on, and the flag is then a one-line change. Two details that are not carried over unchanged from the single-send version, both covered by tests: * timeout now bounds the whole call rather than the wait after the send. Attempts share one budget, so it has to be armed before the first pace — and a caller that asked for 8s should not wait 8s plus however long the rate limiter withheld the request. * a retransmission that fails to send is best-effort. A connected UDP socket reports the ICMP error queued by an earlier send on the next one, and the reader already treats those errnos as advisory; failing the exchange there would make retransmitting less robust than leaving it off. Attempt 0 still raises, since it is the caller's only datagram. Rebased onto the shared MID registry: the empty-ACK and RST matching this originally carried is QuiteYellow#57's now, and QuiteYellow#58 gave the read path the same one-datagram-per-exchange shape, so what remains here is the write attempt loop and the frames that must stop it. Attempt 0 keeps QuiteYellow#51's pace-then-check-then-send ordering exactly; the liveness recheck is skipped only once something has answered, because an answer that beat a dying reader is a write the device confirmed and must not be discarded.
Context
Thank you for clarifying the ownership and landing sequence in #56.
This is the standalone shared MID-registry slice agreed there. Both #36 and
#54 need to correlate tokenless CoAP control messages with pending requests.
Landing that lifecycle once avoids carrying separate read-side and write-side
MID registries before those PRs are rebased.
What changes
preserving the existing token-indexed response dispatch.
that is still owned by a live exchange.
MID-correlated control message.
retransmitting that MID and waits for the token-correlated separate response
under the existing overall deadline.
container, so clearing a stale Block2 response cannot discard the ACK state.
SessionError.or RST.
SessionClosedErrorfromboth
close()and reader-thread teardown.already passed its entry check from registering after the drain.
Scope
This PR intentionally does not include:
changes; or
The existing Block2 duplicate, SZX, ETag, and accumulation semantics remain
unchanged. Moving acknowledgement state outside the response container also
addresses #36 review point 2 without bringing the rest of that PR into this
change.
Validation
python -m pytest -q: 320 passed in a clean Python 3.13 environmentgit diff --checkAfter this lands, I will rebase #36 and address its remaining review points,
then let @mbillow know that #54 is ready to rebase.
Refs #56