bpo-32251: Implement asyncio.BufferedProtocol. #4755
Conversation
1st1
force-pushed
the
get_buffer
branch
2 times, most recently
from
353e0e8 to
4c21a87
Compare
|
The idea is clean, I like it. |
Lib/asyncio/selector_events.py
Outdated
|
|
||
| try: | ||
| buf = self._protocol.get_buffer() | ||
| with memoryview(buf) as mem: |
There was a problem hiding this comment.
You shouldn't need a memoryview here, just pass buf to recv_into.
| return | ||
|
|
||
| try: | ||
| buf = self._protocol.get_buffer() |
There was a problem hiding this comment.
Should this be inside the try...except?
There was a problem hiding this comment.
I think it should. Any unhandled exception in get_buffer() means that the protocol is in an inconsistent state, and that it's safer to close the transport and shut everything down. Speaking of which, we should put buffer_updated and data_received in similar try..except blocks.
|
@asvetlov Andrew, can you please take a look? If you are OK with this PR, I'll add docs & a functional test or two. |
| 'protocol': self._protocol, | ||
| }) | ||
| self._force_close(exc) | ||
| try: |
There was a problem hiding this comment.
Looks like not related to PR subject but I love the change.
There was a problem hiding this comment.
I think that the proactor code hasn't received any love recently, so there're many things we can improve here. I'm just fixing things I see slightly broken.
| @@ -159,6 +168,11 @@ def pause_reading(self): | |||
| if self._closing or self._paused: | |||
| return | |||
| self._paused = True | |||
|
|
|||
| if self._read_fut is not None and not self._read_fut.done(): | |||
There was a problem hiding this comment.
Proactor tricks again?
Not sure if cancelling is what we should do here.
self._read_fut should be set on next resume_reading call maybe? No sure though.
There was a problem hiding this comment.
No, I think this is just a bug. The problem is that when you call pause_reading, the current code doesn't actually pause it. It just sets a _paused flag but leaves the socket in the reading mode. Do you agree, or am I missing something here?
| @@ -170,11 +184,25 @@ def resume_reading(self): | |||
| if self._loop.get_debug(): | |||
| logger.debug("%r resumes reading", self) | |||
|
|
|||
| def _loop_reading(self, fut=None): | |||
| def _loop_reading__on_eof(self): | |||
There was a problem hiding this comment.
Is two underscore in name intentional?
We never use this code style in asyncio
There was a problem hiding this comment.
It's just a bit more readable this way, since it's one "family" of functions (all three of them are combined in one "mega" function). I can replace with one underscore, but I think it's OK this way too. Up to you.
|
Alright, I'll work on adding functional tests then. |
|
Just ran uvloop functional tests with updated asyncio: everything appears to be normal. |
|
@1st1: Please replace |
https://bugs.python.org/issue32251