From f1d6dd1864c368ba4c39d9366241d1688d4d6f03 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Tue, 9 Jun 2026 10:57:30 -0400 Subject: [PATCH 1/8] Ensure `close` and `connection_lost` do not race in Win32 --- serialx/platforms/serial_win32.py | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/serialx/platforms/serial_win32.py b/serialx/platforms/serial_win32.py index 625b170..4e90b84 100644 --- a/serialx/platforms/serial_win32.py +++ b/serialx/platforms/serial_win32.py @@ -484,22 +484,26 @@ def __init__( self._closing: bool = False self._connect_in_progress: bool = False self._connection_made_waiter: asyncio.Future[None] | None = None + self._pending_connection_lost_exc: Exception | None = None def serial_close(self) -> None: - """Close the serial port.""" - - def _close_then_notify() -> None: - assert self._serial is not None - exc = None + """Release the handle off the event loop, then report connection lost.""" + if self._close_future is not None: + return - try: - self._serial.close() - except Exception as e: - exc = e + serial = self._serial + exc = self._pending_connection_lost_exc + self._serial = None + self._handle = None - self._loop.call_soon_threadsafe(self._call_protocol_connection_lost, exc) + if serial is None: + self._call_protocol_connection_lost(exc) + return - self._close_future = self._loop.run_in_executor(None, _close_then_notify) + self._close_future = self._loop.run_in_executor(None, serial.close) + self._close_future.add_done_callback( + lambda _fut: self._call_protocol_connection_lost(exc) + ) def serial_shutdown(self, how: int) -> None: """Shutdown the serial connection.""" @@ -527,8 +531,8 @@ def protocol_connection_made(self, transport: asyncio.Transport) -> None: self._connection_made_waiter.set_result(None) def protocol_connection_lost(self, exc: Exception | None) -> None: - """Forward connection_lost to the protocol.""" - self._call_protocol_connection_lost(exc) + """Stash the connection-lost reason, `serial_close` dispatches it.""" + self._pending_connection_lost_exc = exc def protocol_pause_writing(self) -> None: """Forward pause_writing to the protocol.""" From 574f880906ddaf8c6230b33e6c65e57bc927ed82 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Tue, 9 Jun 2026 11:05:55 -0400 Subject: [PATCH 2/8] Inline `exc = self._pending_connection_lost_exc` --- serialx/platforms/serial_win32.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/serialx/platforms/serial_win32.py b/serialx/platforms/serial_win32.py index 4e90b84..6e0b976 100644 --- a/serialx/platforms/serial_win32.py +++ b/serialx/platforms/serial_win32.py @@ -492,17 +492,18 @@ def serial_close(self) -> None: return serial = self._serial - exc = self._pending_connection_lost_exc self._serial = None self._handle = None if serial is None: - self._call_protocol_connection_lost(exc) + self._call_protocol_connection_lost(self._pending_connection_lost_exc) return self._close_future = self._loop.run_in_executor(None, serial.close) self._close_future.add_done_callback( - lambda _fut: self._call_protocol_connection_lost(exc) + lambda _fut: self._call_protocol_connection_lost( + self._pending_connection_lost_exc + ) ) def serial_shutdown(self, how: int) -> None: From 35234f517214a3c0c65e2c562602183baebb3a72 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Tue, 9 Jun 2026 11:25:35 -0400 Subject: [PATCH 3/8] Add a fast reopen test --- tests/test_async_lifecycle.py | 39 +++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/tests/test_async_lifecycle.py b/tests/test_async_lifecycle.py index 71dd3b8..3742f07 100644 --- a/tests/test_async_lifecycle.py +++ b/tests/test_async_lifecycle.py @@ -138,6 +138,45 @@ async def test_lifecycle_normal_close_callbacks(serial_pair: SerialPair) -> None protocol.assert_clean() +async def test_lifecycle_port_released_before_connection_lost( + serial_pair: SerialPair, +) -> None: + """Test that the underlying port is released before connection_lost fires.""" + loop = asyncio.get_running_loop() + reopen_result: asyncio.Future[BaseSerialTransport] = loop.create_future() + + class ReopenOnLost(RecordingProtocol): + reopen_task: asyncio.Task[None] | None = None + + def connection_lost(self, exc: Exception | None) -> None: + super().connection_lost(exc) + + async def _reopen() -> None: + try: + reopened, _ = await create_serial_connection( + loop, RecordingProtocol, serial_pair.left, baudrate=115200 + ) + except Exception as err: + reopen_result.set_exception(err) + else: + reopen_result.set_result(reopened) + + self.reopen_task = loop.create_task(_reopen()) + + protocol = ReopenOnLost() + transport, _ = await create_serial_connection( + loop, lambda: protocol, serial_pair.left, baudrate=115200 + ) + + transport.close() + await transport.wait_closed() + + reopened = await reopen_result + reopened.close() + await reopened.wait_closed() + protocol.assert_clean() + + async def test_lifecycle_abort_callbacks(serial_pair: SerialPair) -> None: """Connect + abort: traverses INIT -> MADE -> LOST.""" loop = asyncio.get_running_loop() From 0a500741c479fe6692bb9a82f416db37e9767bb7 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Tue, 9 Jun 2026 11:36:37 -0400 Subject: [PATCH 4/8] Run all lifecycle tests with an eager task factory as well --- tests/test_async_lifecycle.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/test_async_lifecycle.py b/tests/test_async_lifecycle.py index 3742f07..1a8d7a6 100644 --- a/tests/test_async_lifecycle.py +++ b/tests/test_async_lifecycle.py @@ -116,6 +116,16 @@ def total_received(self) -> bytes: return b"".join(self.data_received_chunks) +@pytest.fixture(autouse=True, params=["lazy_tasks", "eager_tasks"]) +async def task_factory(request: pytest.FixtureRequest) -> None: + """Run every lifecycle test under both the default and eager task factories.""" + if request.param == "eager_tasks": + if sys.version_info < (3, 12): + pytest.skip("Eager task factory requires Python 3.12+") + + asyncio.get_running_loop().set_task_factory(asyncio.eager_task_factory) + + # --- Successful lifecycle: callbacks fire exactly once --- From 1ba932f3f78121d6d830e0c6d7d8333af30f2dfe Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Tue, 9 Jun 2026 11:39:17 -0400 Subject: [PATCH 5/8] Clean up --- tests/test_async_lifecycle.py | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/tests/test_async_lifecycle.py b/tests/test_async_lifecycle.py index 1a8d7a6..602c1af 100644 --- a/tests/test_async_lifecycle.py +++ b/tests/test_async_lifecycle.py @@ -153,24 +153,20 @@ async def test_lifecycle_port_released_before_connection_lost( ) -> None: """Test that the underlying port is released before connection_lost fires.""" loop = asyncio.get_running_loop() - reopen_result: asyncio.Future[BaseSerialTransport] = loop.create_future() class ReopenOnLost(RecordingProtocol): - reopen_task: asyncio.Task[None] | None = None + reopen_task: asyncio.Task[BaseSerialTransport] | None = None def connection_lost(self, exc: Exception | None) -> None: super().connection_lost(exc) - async def _reopen() -> None: - try: - reopened, _ = await create_serial_connection( - loop, RecordingProtocol, serial_pair.left, baudrate=115200 - ) - except Exception as err: - reopen_result.set_exception(err) - else: - reopen_result.set_result(reopened) + async def _reopen() -> BaseSerialTransport: + reopened, _ = await create_serial_connection( + loop, RecordingProtocol, serial_pair.left, baudrate=115200 + ) + return reopened + # Reopen immediately, eager tasks speed this race up even more self.reopen_task = loop.create_task(_reopen()) protocol = ReopenOnLost() @@ -181,7 +177,8 @@ async def _reopen() -> None: transport.close() await transport.wait_closed() - reopened = await reopen_result + assert protocol.reopen_task is not None + reopened = await protocol.reopen_task reopened.close() await reopened.wait_closed() protocol.assert_clean() From ca722427fbb2395483500e1471b12b1988021c54 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Tue, 9 Jun 2026 11:42:28 -0400 Subject: [PATCH 6/8] Skip pyodide --- tests/test_async_lifecycle.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_async_lifecycle.py b/tests/test_async_lifecycle.py index 602c1af..aa7e63a 100644 --- a/tests/test_async_lifecycle.py +++ b/tests/test_async_lifecycle.py @@ -122,6 +122,8 @@ async def task_factory(request: pytest.FixtureRequest) -> None: if request.param == "eager_tasks": if sys.version_info < (3, 12): pytest.skip("Eager task factory requires Python 3.12+") + if sys.platform == "emscripten": + pytest.skip("Pyodide's WebLoop does not support custom task factories") asyncio.get_running_loop().set_task_factory(asyncio.eager_task_factory) From b8d1517c87a2be6e40bb7bd28ba280590118c4d6 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Tue, 9 Jun 2026 12:05:51 -0400 Subject: [PATCH 7/8] Rewrite test to do slow close instead --- tests/test_async_lifecycle.py | 40 ++++++++++++++++------------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/tests/test_async_lifecycle.py b/tests/test_async_lifecycle.py index aa7e63a..9ca6a18 100644 --- a/tests/test_async_lifecycle.py +++ b/tests/test_async_lifecycle.py @@ -153,36 +153,32 @@ async def test_lifecycle_normal_close_callbacks(serial_pair: SerialPair) -> None async def test_lifecycle_port_released_before_connection_lost( serial_pair: SerialPair, ) -> None: - """Test that the underlying port is released before connection_lost fires.""" + """connection_lost must not fire until the port-releasing syscall returns.""" loop = asyncio.get_running_loop() + protocol = RecordingProtocol() - class ReopenOnLost(RecordingProtocol): - reopen_task: asyncio.Task[BaseSerialTransport] | None = None - - def connection_lost(self, exc: Exception | None) -> None: - super().connection_lost(exc) - - async def _reopen() -> BaseSerialTransport: - reopened, _ = await create_serial_connection( - loop, RecordingProtocol, serial_pair.left, baudrate=115200 - ) - return reopened - - # Reopen immediately, eager tasks speed this race up even more - self.reopen_task = loop.create_task(_reopen()) + close_targets = ["os.close"] + if sys.platform == "win32": + close_targets.append("serialx.platforms.serial_win32.CloseHandle") - protocol = ReopenOnLost() transport, _ = await create_serial_connection( loop, lambda: protocol, serial_pair.left, baudrate=115200 ) - transport.close() - await transport.wait_closed() + with patch_slow(*close_targets) as (started, proceed, _completed): + transport.close() + + if not await loop.run_in_executor(None, started.wait, 1.0): + pytest.skip("Backend close path does not go through a patched syscall") + + # The releasing syscall is mid-flight, so the handle is still held. The + # protocol must not have been told the connection is lost. + assert protocol.state is ProtocolState.MADE - assert protocol.reopen_task is not None - reopened = await protocol.reopen_task - reopened.close() - await reopened.wait_closed() + proceed.set() + + await transport.wait_closed() + assert protocol.state is ProtocolState.LOST # type:ignore[comparison-overlap] protocol.assert_clean() From 4b21dd2ba017ed3ca23989e75d583d154a1878de Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Tue, 9 Jun 2026 13:13:12 -0400 Subject: [PATCH 8/8] Address review feedback --- serialx/platforms/serial_win32.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/serialx/platforms/serial_win32.py b/serialx/platforms/serial_win32.py index 6e0b976..d8c046e 100644 --- a/serialx/platforms/serial_win32.py +++ b/serialx/platforms/serial_win32.py @@ -500,11 +500,21 @@ def serial_close(self) -> None: return self._close_future = self._loop.run_in_executor(None, serial.close) - self._close_future.add_done_callback( - lambda _fut: self._call_protocol_connection_lost( - self._pending_connection_lost_exc + self._close_future.add_done_callback(self._on_serial_closed) + + def _on_serial_closed(self, fut: asyncio.Future[None]) -> None: + # Consume the future's exception so it does not surface later as a noisy warning + if (exc := fut.exception()) is not None: + self._loop.call_exception_handler( + { + "message": "Unhandled exception while closing the serial port", + "exception": exc, + "transport": self, + "protocol": self._protocol, + } ) - ) + + self._call_protocol_connection_lost(self._pending_connection_lost_exc) def serial_shutdown(self, how: int) -> None: """Shutdown the serial connection."""