bpo-32391: Implement StreamWriter.wait_closed() #5281

asvetlov · 2018-01-23T08:51:39Z

Draft, need tests

https://bugs.python.org/issue32391

…io-streams

1st1 · 2018-01-23T16:07:45Z

Please go ahead and add the tests.

asvetlov · 2018-01-24T13:31:28Z

Done, please review

1st1 · 2018-01-24T16:33:10Z

Doc/library/asyncio-stream.rst

@@ -201,6 +201,16 @@ StreamWriter

      Close the transport: see :meth:`BaseTransport.close`.

+   .. coroutinemethod:: wait_closed()
+
+      Wait for writer closing.


-> "Wait until the writer is closed."

1st1 · 2018-01-24T16:39:46Z

Doc/library/asyncio-stream.rst

+
+      Should be called after :meth:`close` to waiting for finishing
+      all writer activities (:meth:`BaseProtocol.connection_lost`
+      callback call actually).


-> "Should be called after :meth:close to wait until the underlying
connection (and the associated transport/protocol pair) is closed."

1st1 · 2018-01-24T16:40:37Z

Lib/asyncio/streams.py

+    def __del__(self):
+        # Prevent reports about unhandled exceptions.
+        # Better than self._closed._log_traceback = False hack
+        if self._closed.done():


if self._closed.done() and not self._closed.cancelled()

Ideally need a test case for the cancelled check.

Fixed.
Don you insist on test?
I see no way to make it steady without mocking everything.

If it's so hard to hit this then it's OK. I don't like super complex mocked tests anyways.

1st1 · 2018-01-24T16:41:30Z

Lib/asyncio/streams.py

+
+    async def wait_closed(self):
+        if self._transport is not None:
+            await self._protocol._closed


When is the transport None? Can you add a comment?

Actually I have no idea, just copied the line from drain.
Looking on source code there is no line for setting transport to None.
Let's drop the check from both methods.

1st1 · 2018-01-24T16:41:58Z

Lib/asyncio/streams.py

@@ -303,6 +315,13 @@ def can_write_eof(self):
    def close(self):
        return self._transport.close()

+    def is_closing(self):


Is this a new API method? Please document it if it is.

Forgot about the change - but exposing the method makes sense

I think it's a bit too low-level. Can you imagine a good use case for async/await code that uses writer and reader?

StreamWriter exposes all Transport methods except flow control related.
is_closing() is not about flow control, I see no reason to not implement it.

OK, go ahead.

1st1 · 2018-01-24T16:42:18Z

Misc/NEWS.d/next/Library/2018-01-24-15-20-12.bpo-32391.0f8MY9.rst

@@ -0,0 +1 @@
+Implement :meth:`asyncio.StreamWriter.wait_closed` method


And is_closing() ?

asvetlov · 2018-01-24T20:40:14Z

Please review again.

1st1 · 2018-01-24T20:42:34Z

Misc/NEWS.d/next/Library/2018-01-24-15-20-12.bpo-32391.0f8MY9.rst

@@ -0,0 +1 @@
+Implement :meth:`asyncio.StreamWriter.wait_closed` and :meth:`asyncio.StreamWriter.is_closing` methods


One last note: I don't think you want to use ReST formatting in NEWS entries. This would read better:

Implement wait_closed() and is_closing() methods for StreamWriter.

There is no standard for blurb text, the format of messages is mixed.
Some people use ReST, others prefer plain text.
If you briefly look on these files you'll see both cases.

I have no personal preference.

I don't really care.

1st1 · 2018-01-24T20:43:31Z

Lib/test/test_asyncio/test_streams.py

+            data = self.loop.run_until_complete(f)
+            self.assertTrue(data.endswith(b'\r\n\r\nTest message'))
+            wr.close()
+            self.loop.run_until_complete(wr.wait_closed())


Add assertTrue(wr.is_closing())

1st1 · 2018-01-24T20:43:49Z

Lib/test/test_asyncio/test_streams.py

+            f = rd.read()
+            data = self.loop.run_until_complete(f)
+            self.assertTrue(data.endswith(b'\r\n\r\nTest message'))
+            wr.close()


Add assertFalse(wr.is_closing()) before wr.close()

1st1 · 2018-01-24T22:19:43Z

LGTM.

asvetlov added 5 commits Dec 21, 2017

Draft

2d7c3b7

Merge branch 'master' into asyncio-streams

67e3be3

Move _closed into protocol

a363e96

Merge branch 'master' of https://github.com/python/cpython into async…

5fee054

…io-streams

Merge branch 'master' into asyncio-streams

8e3009c

asvetlov requested a review from 1st1 as a code owner Jan 23, 2018

the-knights-who-say-ni added the CLA signed label Jan 23, 2018

bedevere-bot added the awaiting merge label Jan 23, 2018

asvetlov added 5 commits Jan 24, 2018

Merge branch 'master' into asyncio-streams

542ad22

Rename test suite

7d05558

Add tests

445c220

Add NEWS

23ec87f

Add docs

2a9de5b

1st1 reviewed Jan 24, 2018

View changes

asvetlov added 3 commits Jan 24, 2018

Fix docs

ce4c334

Fix notes

0b4cd2f

Update docs

1cad29d

1st1 reviewed Jan 24, 2018

View changes

1st1 approved these changes Jan 24, 2018

View changes

1st1 reviewed Jan 24, 2018

View changes

Test is_closing()

8254213

asvetlov merged commit fe133aa into python:master Jan 24, 2018

bedevere-bot removed the awaiting merge label Jan 24, 2018

asvetlov deleted the asyncio-streams branch Jan 24, 2018

bpo-32391: Implement StreamWriter.wait_closed() #5281

bpo-32391: Implement StreamWriter.wait_closed() #5281

asvetlov commented Jan 23, 2018 •

edited by bedevere-bot

1st1 commented Jan 23, 2018

asvetlov commented Jan 24, 2018

1st1 Jan 24, 2018

1st1 Jan 24, 2018

1st1 Jan 24, 2018

1st1 Jan 24, 2018

asvetlov Jan 24, 2018

1st1 Jan 24, 2018

1st1 Jan 24, 2018

asvetlov Jan 24, 2018

1st1 Jan 24, 2018

asvetlov Jan 24, 2018

1st1 Jan 24, 2018

asvetlov Jan 24, 2018

1st1 Jan 24, 2018

1st1 Jan 24, 2018

asvetlov commented Jan 24, 2018

1st1 Jan 24, 2018

asvetlov Jan 24, 2018

1st1 Jan 24, 2018

1st1 Jan 24, 2018

1st1 Jan 24, 2018 •

edited

1st1 commented Jan 24, 2018

		@@ -0,0 +1 @@
		Implement :meth:`asyncio.StreamWriter.wait_closed` method

bpo-32391: Implement StreamWriter.wait_closed() #5281

bpo-32391: Implement StreamWriter.wait_closed() #5281

Conversation

asvetlov commented Jan 23, 2018 • edited by bedevere-bot

1st1 commented Jan 23, 2018

asvetlov commented Jan 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvetlov commented Jan 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1st1 Jan 24, 2018 • edited

Choose a reason for hiding this comment

1st1 commented Jan 24, 2018

asvetlov commented Jan 23, 2018 •

edited by bedevere-bot

1st1 Jan 24, 2018 •

edited