feat(pool): transparent reconnect on stale connection - #55
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements transparent automatic reconnection in ConnectionPool for stale persistent connections, retrying a failed command once on a new connection before raising an error. It introduces proxy classes ReconnectingStreamReader and ReconnectingStreamWriter to wrap the underlying stream reader and writer, along with corresponding documentation and regression tests. The review feedback suggests several improvements: unpacking is_reused directly from _checkout instead of using getattr, wrapping asyncio.TimeoutError in DLightTimeoutError during drain() for consistency, and simplifying the exception handling in ReconnectingStreamReader.readuntil by removing asyncio.LimitOverrunError from the caught exceptions tuple.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| reader, writer = await self._checkout(key, host, port, ssl, connect_timeout) | ||
| is_reused = getattr(writer, "_is_reused", False) | ||
| state = ReconnectingState(self, host, port, ssl, connect_timeout, is_reused, reader, writer) |
There was a problem hiding this comment.
Update the call to _checkout to unpack the new 3-tuple return value containing is_reused, avoiding the need to retrieve it via getattr.
| reader, writer = await self._checkout(key, host, port, ssl, connect_timeout) | |
| is_reused = getattr(writer, "_is_reused", False) | |
| state = ReconnectingState(self, host, port, ssl, connect_timeout, is_reused, reader, writer) | |
| reader, writer, is_reused = await self._checkout(key, host, port, ssl, connect_timeout) | |
| state = ReconnectingState(self, host, port, ssl, connect_timeout, is_reused, reader, writer) |
| if self.write_buffer: | ||
| try: | ||
| self.writer.write(self.write_buffer) | ||
| await asyncio.wait_for(self.writer.drain(), timeout=self.connect_timeout) | ||
| except Exception as e: | ||
| _LOGGER.error(f"Transparent reconnect failed during write/drain: {e}") | ||
| raise DLightConnectionError( | ||
| f"Network error during transparent reconnect write/drain: {e}" | ||
| ) from e |
There was a problem hiding this comment.
If the drain() operation times out during a transparent reconnect, it raises asyncio.TimeoutError. This should be wrapped in DLightTimeoutError for consistency with the rest of the library's timeout handling (e.g., in client.py), rather than being caught by the generic except Exception block and wrapped in DLightConnectionError.
| if self.write_buffer: | |
| try: | |
| self.writer.write(self.write_buffer) | |
| await asyncio.wait_for(self.writer.drain(), timeout=self.connect_timeout) | |
| except Exception as e: | |
| _LOGGER.error(f"Transparent reconnect failed during write/drain: {e}") | |
| raise DLightConnectionError( | |
| f"Network error during transparent reconnect write/drain: {e}" | |
| ) from e | |
| if self.write_buffer: | |
| try: | |
| self.writer.write(self.write_buffer) | |
| await asyncio.wait_for(self.writer.drain(), timeout=self.connect_timeout) | |
| except asyncio.TimeoutError as e: | |
| _LOGGER.error(f"Transparent reconnect timed out during write/drain: {e}") | |
| raise DLightTimeoutError( | |
| f"Timeout during transparent reconnect write/drain" | |
| ) from e | |
| except Exception as e: | |
| _LOGGER.error(f"Transparent reconnect failed during write/drain: {e}") | |
| raise DLightConnectionError( | |
| f"Network error during transparent reconnect write/drain: {e}" | |
| ) from e |
| except (OSError, asyncio.IncompleteReadError, asyncio.LimitOverrunError) as e: | ||
| if not isinstance(e, asyncio.CancelledError) and not isinstance(e, asyncio.LimitOverrunError): |
There was a problem hiding this comment.
In ReconnectingStreamReader.readuntil, asyncio.LimitOverrunError is caught in the except block, only to be explicitly checked and propagated without retrying. Since LimitOverrunError is not a subclass of OSError or IncompleteReadError, omitting it from the except tuple will allow it to propagate naturally, simplifying the code.
| except (OSError, asyncio.IncompleteReadError, asyncio.LimitOverrunError) as e: | |
| if not isinstance(e, asyncio.CancelledError) and not isinstance(e, asyncio.LimitOverrunError): | |
| except (OSError, asyncio.IncompleteReadError) as e: | |
| if not isinstance(e, asyncio.CancelledError): |
57e0529 to
d12e420
Compare
d12e420 to
1614272
Compare
Addresses #36.
Problem
When a persistent TCP connection goes stale (e.g. because of a device reboot or network blip), the connection pool raised
DLightConnectionErrorand required the caller to handle reconnecting and retrying manually.Solution
ReconnectingState,ReconnectingStreamReader, andReconnectingStreamWriterproxy wrappers inside the privateConnectionPool(_pool.py).OSErrororasyncio.IncompleteReadError) during read/write/drain operations:ConnectionPool's docstring.docs/architecture.md,docs/ARCHITECTURE.md, anddocs/user-guide/connections.md.Tests
test_transparent_reconnect_on_stale_connectionto cover connection drops mid-session usingFakeDLightServer.test_no_transparent_reconnect_on_fresh_connection_failureto verify that new connections do not retry on failure.