From ca0511442db173714a8d056f51a54245023bf243 Mon Sep 17 00:00:00 2001 From: Javad Asgari Shafique Date: Sun, 12 May 2024 03:01:22 +0200 Subject: [PATCH 1/5] gh-118950: Let _SSLProtocolTransport.is_closing reflect SSLProtocol internal transport closing state so that StreamWriter.drain will invoke sleep(0) which calls connection_lost and correctly notifies waiters of connection lost. (#118950) --- Lib/asyncio/sslproto.py | 5 ++++- .../2024-05-12-03-10-36.gh-issue-118950.5Wc4vp.rst | 3 +++ 2 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2024-05-12-03-10-36.gh-issue-118950.5Wc4vp.rst diff --git a/Lib/asyncio/sslproto.py b/Lib/asyncio/sslproto.py index fa99d4533aa0a6..74c5f0d5ca0609 100644 --- a/Lib/asyncio/sslproto.py +++ b/Lib/asyncio/sslproto.py @@ -101,7 +101,7 @@ def get_protocol(self): return self._ssl_protocol._app_protocol def is_closing(self): - return self._closed + return self._closed or self._ssl_protocol._is_transport_closing() def close(self): """Close the transport. @@ -379,6 +379,9 @@ def _get_app_transport(self): self._app_transport_created = True return self._app_transport + def _is_transport_closing(self): + return self._transport is not None and self._transport.is_closing() + def connection_made(self, transport): """Called when the low-level connection is made. diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-05-12-03-10-36.gh-issue-118950.5Wc4vp.rst b/Misc/NEWS.d/next/Core and Builtins/2024-05-12-03-10-36.gh-issue-118950.5Wc4vp.rst new file mode 100644 index 00000000000000..eb72db341c2d90 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2024-05-12-03-10-36.gh-issue-118950.5Wc4vp.rst @@ -0,0 +1,3 @@ +Let _SSLProtocolTransport.is_closing reflect SSLProtocol internal transport +closing state so that StreamWriter.drain will invoke sleep(0) which calls +connection_lost and correctly notifies waiters of connection lost. From 9b5f9c89018953a51f18616df2d8fca8d98741d8 Mon Sep 17 00:00:00 2001 From: Javad Asgari Shafique Date: Thu, 15 Aug 2024 00:04:46 +0200 Subject: [PATCH 2/5] Add test for gh-118950 --- Lib/test/test_asyncio/test_sslproto.py | 43 ++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/Lib/test/test_asyncio/test_sslproto.py b/Lib/test/test_asyncio/test_sslproto.py index f5f0afeab51c9e..f72ef272d36132 100644 --- a/Lib/test/test_asyncio/test_sslproto.py +++ b/Lib/test/test_asyncio/test_sslproto.py @@ -16,6 +16,7 @@ from asyncio import log from asyncio import protocols from asyncio import sslproto +from asyncio import selector_events from test.test_asyncio import utils as test_utils from test.test_asyncio import functional as func_tests @@ -109,6 +110,48 @@ def test_connection_lost(self): test_utils.run_briefly(self.loop) self.assertIsInstance(waiter.exception(), ConnectionAbortedError) + def test_connection_lost_when_busy(self): + sock = mock.Mock() + sock.fileno = mock.Mock(return_value=12345) + sock.send = mock.Mock(side_effect=BrokenPipeError) + + # construct StreamWriter chain that contains loop dependant logic this emulates that + # _make_ssl_transport() does in BaseSelectorEventLoop + reader = asyncio.StreamReader(limit=2 ** 16, loop=self.loop) + protocol = asyncio.StreamReaderProtocol(reader, loop=self.loop) + ssl_proto = self.ssl_protocol(proto=protocol) + + # emulate reading decompressed data + sslobj = mock.Mock() + sslobj.read.side_effect = ssl.SSLWantReadError + sslobj.write.side_effect = ssl.SSLWantReadError + ssl_proto._sslobj = sslobj + + # emulate outgoing data + data = b'An interesting message' + + outgoing = mock.Mock() + outgoing.read = mock.Mock(return_value=data) + outgoing.pending = len(data) + ssl_proto._outgoing = outgoing + + # use correct socket transport to initialize the SSLProtocol + selector_events._SelectorSocketTransport(self.loop, sock, ssl_proto) + transport = ssl_proto._app_transport + writer = asyncio.StreamWriter(transport, protocol, reader, self.loop) + + # Write data to the transport n times in a task that blocks the + # asyncio event loop from a user perspective. + async def _write_loop(n): + for i in range(n): + writer.write(data) + await writer.drain() + + # The test is successful if we raise the error the next time + # we try to write to the transport. + with self.assertRaises(ConnectionResetError): + self.loop.run_until_complete(_write_loop(2)) + def test_close_during_handshake(self): # bpo-29743 Closing transport during handshake process leaks socket waiter = self.loop.create_future() From d4b2c8ccfa57bc41eac827ba67b25e8666a1e760 Mon Sep 17 00:00:00 2001 From: Javad Asgari Shafique Date: Thu, 15 Aug 2024 00:39:05 +0200 Subject: [PATCH 3/5] Use correct event loop primitive for test in sslproto --- Lib/test/test_asyncio/test_sslproto.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_asyncio/test_sslproto.py b/Lib/test/test_asyncio/test_sslproto.py index f72ef272d36132..7d8ed456fc6ed3 100644 --- a/Lib/test/test_asyncio/test_sslproto.py +++ b/Lib/test/test_asyncio/test_sslproto.py @@ -16,7 +16,6 @@ from asyncio import log from asyncio import protocols from asyncio import sslproto -from asyncio import selector_events from test.test_asyncio import utils as test_utils from test.test_asyncio import functional as func_tests @@ -111,12 +110,14 @@ def test_connection_lost(self): self.assertIsInstance(waiter.exception(), ConnectionAbortedError) def test_connection_lost_when_busy(self): + # gh-118950: SSLProtocol.connection_lost not being called when OSError + # is thrown on asyncio.write. sock = mock.Mock() sock.fileno = mock.Mock(return_value=12345) sock.send = mock.Mock(side_effect=BrokenPipeError) - # construct StreamWriter chain that contains loop dependant logic this emulates that - # _make_ssl_transport() does in BaseSelectorEventLoop + # construct StreamWriter chain that contains loop dependant logic this emulates + # what _make_ssl_transport() does in BaseSelectorEventLoop reader = asyncio.StreamReader(limit=2 ** 16, loop=self.loop) protocol = asyncio.StreamReaderProtocol(reader, loop=self.loop) ssl_proto = self.ssl_protocol(proto=protocol) @@ -136,7 +137,8 @@ def test_connection_lost_when_busy(self): ssl_proto._outgoing = outgoing # use correct socket transport to initialize the SSLProtocol - selector_events._SelectorSocketTransport(self.loop, sock, ssl_proto) + self.loop._make_socket_transport(sock, ssl_proto) + transport = ssl_proto._app_transport writer = asyncio.StreamWriter(transport, protocol, reader, self.loop) From c35c59909da668f0d93c1c2f4ced9e97d859ccc0 Mon Sep 17 00:00:00 2001 From: Kumar Aditya Date: Thu, 24 Oct 2024 20:47:28 +0530 Subject: [PATCH 4/5] tweak test --- Lib/test/test_asyncio/test_sslproto.py | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/Lib/test/test_asyncio/test_sslproto.py b/Lib/test/test_asyncio/test_sslproto.py index 7d8ed456fc6ed3..761904c5146b6a 100644 --- a/Lib/test/test_asyncio/test_sslproto.py +++ b/Lib/test/test_asyncio/test_sslproto.py @@ -142,17 +142,20 @@ def test_connection_lost_when_busy(self): transport = ssl_proto._app_transport writer = asyncio.StreamWriter(transport, protocol, reader, self.loop) - # Write data to the transport n times in a task that blocks the - # asyncio event loop from a user perspective. - async def _write_loop(n): - for i in range(n): + async def main(): + # writes data to transport + async def write(): writer.write(data) await writer.drain() - # The test is successful if we raise the error the next time - # we try to write to the transport. - with self.assertRaises(ConnectionResetError): - self.loop.run_until_complete(_write_loop(2)) + # try to write for the first time + await write() + # try to write for the second time, this raises as the connection_lost + # callback should be done with error + with self.assertRaises(ConnectionResetError): + await write() + + self.loop.run_until_complete(main()) def test_close_during_handshake(self): # bpo-29743 Closing transport during handshake process leaks socket From 158899d005678f4626d5f860ef3c92ba9ba8d048 Mon Sep 17 00:00:00 2001 From: Kumar Aditya Date: Thu, 24 Oct 2024 21:26:32 +0530 Subject: [PATCH 5/5] Update Misc/NEWS.d/next/Core and Builtins/2024-05-12-03-10-36.gh-issue-118950.5Wc4vp.rst --- .../2024-05-12-03-10-36.gh-issue-118950.5Wc4vp.rst | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-05-12-03-10-36.gh-issue-118950.5Wc4vp.rst b/Misc/NEWS.d/next/Core and Builtins/2024-05-12-03-10-36.gh-issue-118950.5Wc4vp.rst index eb72db341c2d90..82be975f4d808d 100644 --- a/Misc/NEWS.d/next/Core and Builtins/2024-05-12-03-10-36.gh-issue-118950.5Wc4vp.rst +++ b/Misc/NEWS.d/next/Core and Builtins/2024-05-12-03-10-36.gh-issue-118950.5Wc4vp.rst @@ -1,3 +1 @@ -Let _SSLProtocolTransport.is_closing reflect SSLProtocol internal transport -closing state so that StreamWriter.drain will invoke sleep(0) which calls -connection_lost and correctly notifies waiters of connection lost. +Fix bug where SSLProtocol.connection_lost wasn't getting called when OSError was thrown on writing to socket.