bpo-32391: Implement StreamWriter.wait_closed()#5281
bpo-32391: Implement StreamWriter.wait_closed()#5281asvetlov merged 14 commits intopython:masterfrom
Conversation
|
Please go ahead and add the tests. |
|
Done, please review |
|
|
||
| .. coroutinemethod:: wait_closed() | ||
|
|
||
| Wait for writer closing. |
There was a problem hiding this comment.
-> "Wait until the writer is closed."
|
|
||
| Should be called after :meth:`close` to waiting for finishing | ||
| all writer activities (:meth:`BaseProtocol.connection_lost` | ||
| callback call actually). |
There was a problem hiding this comment.
-> "Should be called after :meth:close to wait until the underlying
connection (and the associated transport/protocol pair) is closed."
| def __del__(self): | ||
| # Prevent reports about unhandled exceptions. | ||
| # Better than self._closed._log_traceback = False hack | ||
| if self._closed.done(): |
There was a problem hiding this comment.
if self._closed.done() and not self._closed.cancelled()
There was a problem hiding this comment.
Ideally need a test case for the cancelled check.
There was a problem hiding this comment.
Fixed.
Don you insist on test?
I see no way to make it steady without mocking everything.
There was a problem hiding this comment.
If it's so hard to hit this then it's OK. I don't like super complex mocked tests anyways.
|
|
||
| async def wait_closed(self): | ||
| if self._transport is not None: | ||
| await self._protocol._closed |
There was a problem hiding this comment.
When is the transport None? Can you add a comment?
There was a problem hiding this 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.
| def close(self): | ||
| return self._transport.close() | ||
|
|
||
| def is_closing(self): |
There was a problem hiding this comment.
Is this a new API method? Please document it if it is.
There was a problem hiding this comment.
Forgot about the change - but exposing the method makes sense
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
StreamWriter exposes all Transport methods except flow control related.
is_closing() is not about flow control, I see no reason to not implement it.
| @@ -0,0 +1 @@ | |||
| Implement :meth:`asyncio.StreamWriter.wait_closed` method | |||
|
Please review again. |
| @@ -0,0 +1 @@ | |||
| Implement :meth:`asyncio.StreamWriter.wait_closed` and :meth:`asyncio.StreamWriter.is_closing` methods | |||
There was a problem hiding this comment.
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 was a problem hiding this comment.
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.
| 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()) |
| f = rd.read() | ||
| data = self.loop.run_until_complete(f) | ||
| self.assertTrue(data.endswith(b'\r\n\r\nTest message')) | ||
| wr.close() |
There was a problem hiding this comment.
Add assertFalse(wr.is_closing()) before wr.close()
|
LGTM. |
Draft, need tests
https://bugs.python.org/issue32391