Fix the issues found in the review of 1.0.3 - #9
Merged
Merged
Conversation
A fourth review, of 1.0.3, drove the real socket path that the test suite fakes and found two things the original library did that this one had lost: a socket that goes bad is replaced, and a hostname that does not resolve fails at once. This fixes both, carries one device fix from upstream, and takes the small items along. Tested on 3.13 and 3.14, 269 tests under -X dev with ResourceWarning as an error, and live against an RM4 Pro including a forced socket failure and a bad hostname. Technical details: - _Protocol.error_received and connection_lost put the error on the receive queue, so a waiting request fails at once with that OSError (send failures: no route, address gone) instead of waiting out the timeout. ICMP port unreachable is logged and treated as silence, which is what the original's unconnected socket saw, so the timeout decides as before. - A request that fails for a network reason drops the endpoint; the next call opens a fresh socket, as the original did by opening one per call. - scan(), ping() and send_setup_packet() resolve the destination once with loop.getaddrinfo and let socket.gaierror propagate; a send failure in ping() and setup() is raised. Nothing resolves on the loop any more. - a2._send uses the SP4/LB1 frame layout (length 12, four-byte data length). The bytes match upstream mjg59#826, tested by its author on a real A2; the one oracle case re-recorded on purpose and noted in harness.py. - xdiscover() closes the scan() generator it wraps; captured_at is excluded from CapturedSignal equality; async generator functions are annotated AsyncGenerator (mypy 40 to 37); TICK docstring matches the README. - tests/test_loopback.py drives the real datagram endpoint, including 20 concurrent requests and the ICMP path on loopback. - README: errors discover()/hello() raise, the socket drop on failure, where CaptureInProgressError can come from. - Version 1.0.4. Co-authored-by: kanshurichard <29994770+kanshurichard@users.noreply.github.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
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.3 reviewed again, this time by someone who drove the real network path our tests fake. They found two things.
First, if a device's network socket went bad (the interface bounced, the host got a new address), the library kept using the dead socket and every request timed out until the device was reloaded. The original opened a fresh socket per call, so it healed on its own. Second, a hostname that did not resolve made the library wait out the full timeout and, for pings, silently send nothing, where the original failed at once with the error Home Assistant maps to "invalid host".
Both are fixed, and we carried a fix from an open upstream pull request for the A2 air quality sensor, which its author tested on a real unit, but we have no way to confirm. Tested on Python 3.13 and 3.14, 269 tests, plus a live run against an RM4 Pro that included a forced socket failure and a bad hostname.
Coding assistant's notes on this change: