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
There was a problem hiding this comment.
You shouldn't need a memoryview here, just pass buf to recv_into.
Lib/asyncio/selector_events.py
Outdated
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. |
asvetlov
left a comment
asvetlov
left a comment
There was a problem hiding this comment.
Looks good in general but I have a couple minor questions.
Functional tests with reduced SO_RECVBUF size would be great!
Lib/asyncio/proactor_events.py
Outdated
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.
Lib/asyncio/proactor_events.py
Outdated
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?
Lib/asyncio/proactor_events.py
Outdated
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 |
5 participants
https://bugs.python.org/issue32251