From 273e1e74a9656b3e45cbc67b284457e730b38ea2 Mon Sep 17 00:00:00 2001 From: Ali-Akber Saifee Date: Tue, 11 Apr 2023 15:59:03 -0700 Subject: [PATCH 1/5] Add explicit tests for writelines Includes two failing tests that demonstrate a bug where if the data passed to writelines can't be sent in one shot, the remaining buffer isn't written since there is no registered write handler --- Lib/test/test_asyncio/test_selector_events.py | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/Lib/test/test_asyncio/test_selector_events.py b/Lib/test/test_asyncio/test_selector_events.py index 921c98a2702d76..08e8f00ac05553 100644 --- a/Lib/test/test_asyncio/test_selector_events.py +++ b/Lib/test/test_asyncio/test_selector_events.py @@ -747,6 +747,46 @@ def test_write_sendmsg_no_data(self): self.assertFalse(self.sock.sendmsg.called) self.assertEqual(list_to_buffer([b'data']), transport._buffer) + def test_writelines_sendmsg_full(self): + data = memoryview(b'data') + self.sock.sendmsg = mock.Mock() + self.sock.sendmsg.return_value = len(data) + + transport = self.socket_transport(sendmsg=True) + transport.writelines([data]) + self.assertTrue(self.sock.sendmsg.called) + self.assertFalse(self.loop.writers) + + def test_writelines_sendmsg_partial(self): + data = memoryview(b'data') + self.sock.sendmsg = mock.Mock() + self.sock.sendmsg.return_value = 2 + + transport = self.socket_transport(sendmsg=True) + transport.writelines([data]) + self.assertTrue(self.sock.sendmsg.called) + self.assertTrue(self.loop.writers) + + def test_writelines_send_full(self): + data = memoryview(b'data') + self.sock.send.return_value = len(data) + self.sock.send.fileno.return_value = 7 + + transport = self.socket_transport(sendmsg=False) + transport.writelines([data]) + self.assertTrue(self.sock.send.called) + self.assertFalse(self.loop.writers) + + def test_writelines_send_partial(self): + data = memoryview(b'data') + self.sock.send.return_value = 2 + self.sock.send.fileno.return_value = 7 + + transport = self.socket_transport(sendmsg=False) + transport.writelines([data]) + self.assertTrue(self.sock.send.called) + self.assertTrue(self.loop.writers) + @unittest.skipUnless(selector_events._HAS_SENDMSG, 'no sendmsg') def test_write_sendmsg_full(self): data = memoryview(b'data') From ce03f17a778d91f83684466d471714a51f9b0bde Mon Sep 17 00:00:00 2001 From: Ali-Akber Saifee Date: Tue, 11 Apr 2023 18:18:45 -0700 Subject: [PATCH 2/5] Schedule writer for remaining buffer in writelines If all the data in the buffer can't be written immediately schedule a writer to handle the remaining data. --- Lib/asyncio/selector_events.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Lib/asyncio/selector_events.py b/Lib/asyncio/selector_events.py index de5076a96218e0..3a697129e4c914 100644 --- a/Lib/asyncio/selector_events.py +++ b/Lib/asyncio/selector_events.py @@ -1176,6 +1176,9 @@ def writelines(self, list_of_data): return self._buffer.extend([memoryview(data) for data in list_of_data]) self._write_ready() + # If the entire buffer couldn't be written, register a write handler + if self._buffer: + self._loop._add_writer(self._sock_fd, self._write_ready) def can_write_eof(self): return True From c5a071e3f40cff537d71329ca05b91dcc1dbc6ad Mon Sep 17 00:00:00 2001 From: Ali-Akber Saifee Date: Tue, 11 Apr 2023 19:34:11 -0700 Subject: [PATCH 3/5] Ensure tests using sendmsg are marked with skip When the platform (windows) doesn't support it skip tests for writelines using sendmsg --- Lib/test/test_asyncio/test_selector_events.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_asyncio/test_selector_events.py b/Lib/test/test_asyncio/test_selector_events.py index 08e8f00ac05553..e41341fd26e19e 100644 --- a/Lib/test/test_asyncio/test_selector_events.py +++ b/Lib/test/test_asyncio/test_selector_events.py @@ -747,6 +747,7 @@ def test_write_sendmsg_no_data(self): self.assertFalse(self.sock.sendmsg.called) self.assertEqual(list_to_buffer([b'data']), transport._buffer) + @unittest.skipUnless(selector_events._HAS_SENDMSG, 'no sendmsg') def test_writelines_sendmsg_full(self): data = memoryview(b'data') self.sock.sendmsg = mock.Mock() @@ -757,6 +758,7 @@ def test_writelines_sendmsg_full(self): self.assertTrue(self.sock.sendmsg.called) self.assertFalse(self.loop.writers) + @unittest.skipUnless(selector_events._HAS_SENDMSG, 'no sendmsg') def test_writelines_sendmsg_partial(self): data = memoryview(b'data') self.sock.sendmsg = mock.Mock() @@ -772,7 +774,7 @@ def test_writelines_send_full(self): self.sock.send.return_value = len(data) self.sock.send.fileno.return_value = 7 - transport = self.socket_transport(sendmsg=False) + transport = self.socket_transport() transport.writelines([data]) self.assertTrue(self.sock.send.called) self.assertFalse(self.loop.writers) @@ -782,7 +784,7 @@ def test_writelines_send_partial(self): self.sock.send.return_value = 2 self.sock.send.fileno.return_value = 7 - transport = self.socket_transport(sendmsg=False) + transport = self.socket_transport() transport.writelines([data]) self.assertTrue(self.sock.send.called) self.assertTrue(self.loop.writers) From 7f2b1e7c1feb2ddb3ceedb0b69522101dfa2469e Mon Sep 17 00:00:00 2001 From: Ali-Akber Saifee Date: Wed, 12 Apr 2023 06:05:03 -0700 Subject: [PATCH 4/5] Add NEWS entry --- .../Library/2023-04-12-06-00-02.gh-issue-103462.w6yBlM.rst | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2023-04-12-06-00-02.gh-issue-103462.w6yBlM.rst diff --git a/Misc/NEWS.d/next/Library/2023-04-12-06-00-02.gh-issue-103462.w6yBlM.rst b/Misc/NEWS.d/next/Library/2023-04-12-06-00-02.gh-issue-103462.w6yBlM.rst new file mode 100644 index 00000000000000..23d9fd6ac5a73a --- /dev/null +++ b/Misc/NEWS.d/next/Library/2023-04-12-06-00-02.gh-issue-103462.w6yBlM.rst @@ -0,0 +1,5 @@ +Fixed an issue with using :meth:`writelines` in :mod:`asyncio` to send very +large payloads that exceed the amount of data that can be written in one +call to :meth:`socket.socket.send` or :meth:`socket.socket.sendmsg`, +resulting in the remaining buffer being left unwritten. This is fixed by +registering a writer when the buffer still exists after the first write. From 239a80f35b74f7fc0bd4e35c21ed1013383af176 Mon Sep 17 00:00:00 2001 From: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com> Date: Thu, 13 Apr 2023 09:51:52 +0530 Subject: [PATCH 5/5] Update Misc/NEWS.d/next/Library/2023-04-12-06-00-02.gh-issue-103462.w6yBlM.rst --- .../Library/2023-04-12-06-00-02.gh-issue-103462.w6yBlM.rst | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Misc/NEWS.d/next/Library/2023-04-12-06-00-02.gh-issue-103462.w6yBlM.rst b/Misc/NEWS.d/next/Library/2023-04-12-06-00-02.gh-issue-103462.w6yBlM.rst index 23d9fd6ac5a73a..50758c89cc2856 100644 --- a/Misc/NEWS.d/next/Library/2023-04-12-06-00-02.gh-issue-103462.w6yBlM.rst +++ b/Misc/NEWS.d/next/Library/2023-04-12-06-00-02.gh-issue-103462.w6yBlM.rst @@ -1,5 +1,4 @@ -Fixed an issue with using :meth:`writelines` in :mod:`asyncio` to send very +Fixed an issue with using :meth:`~asyncio.WriteTransport.writelines` in :mod:`asyncio` to send very large payloads that exceed the amount of data that can be written in one call to :meth:`socket.socket.send` or :meth:`socket.socket.sendmsg`, -resulting in the remaining buffer being left unwritten. This is fixed by -registering a writer when the buffer still exists after the first write. +resulting in the remaining buffer being left unwritten.