Fix the issues found in the review of 1.0.4 - #10
Merged
Merged
Conversation
A fifth review, of 1.0.4, found one more place where the connected socket could surface an error the original library never raised, and made a fair case that the capture window claim was more machinery than the job needed. This fixes the first, simplifies the second, and takes two small items along. No change to the wire format or the public API. Tested on 3.13 and 3.14, 272 tests under -X dev with ResourceWarning as an error, oracle fixtures unchanged, and live against an RM4 Pro. Technical details: - EHOSTUNREACH and Windows's ConnectionResetError are treated as silence in _exchange, as ECONNREFUSED already was: logged, the timeout decides, the socket is dropped afterwards. Home Assistant tolerates a timeout for a few polls but marks a device unavailable on the first OSError, and the original's unconnected socket never saw any of these. Measured on the bench before changing anything: neither an on-link address with no host nor an off-subnet one produced the ICMP here, so this is insurance for networks that do. - The window claim is a flag set on first iteration and cleared in the generator's finally (so asyncio's finalizer releases a dropped window), replacing the weakref and ag_frame inspection. Same behaviour, same tests; CaptureInProgressError now always comes from the first iteration. - discover() closes the xdiscover() generator it drains. - A comment explains why the RM Max sits in rmpro (upstream mjg59#838). - README: a device belongs to the event loop it first talks on. - Version 1.0.5.
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.
I had 1.0.4 reviewed a fifth time. The library came through clean, the review found one more way the new socket could report an error the original never did.
The socket change is small. When a device is off, some routers answer for it with a "host unreachable" message. The original library never saw those and simply timed out, and Home Assistant gives a timeout a few tries before marking a device unavailable, but marks it unavailable at once on a socket error. So those messages are now treated as silence, the way the original behaved. I measured this on the bench first: on my network neither case actually produced the message, so this is insurance for networks that do.
The capture change replaces the mechanism that decides whether a capture window is free with a plain flag. Same behaviour, same tests. Tested on Python 3.13 and 3.14, 272 tests, plus a live run against an RM4 Pro.
Coding assistant's notes on this change: