bpo-32622: Implement loop.sendfile() #5271
Conversation
Lib/asyncio/base_events.py
Outdated
|
|
||
| def connection_made(self, transport): | ||
| raise RuntimeError("Broken state, " | ||
| "connection should be established already.") |
There was a problem hiding this comment.
"Invalid state: connection should have been established already"
Lib/asyncio/base_events.py
Outdated
| self._paused = None | ||
|
|
||
| def data_received(self, data): | ||
| raise RuntimeError("Broken state, reading should be paused") |
Lib/asyncio/base_events.py
Outdated
| self._transport = transp | ||
| self._proto = transp.get_protocol() | ||
| self._resume_reading = transp.is_reading() | ||
| self._write_paused = transp._protocol_paused |
There was a problem hiding this comment.
I'd call these '_should_resume_reading' and '_should_resume_writing'
Lib/asyncio/base_events.py
Outdated
| if mode is constants._SendfileMode.UNSUPPORTED: | ||
| raise RuntimeError( | ||
| f"sendfile is not supported for transport {transport!r}") | ||
| if mode is constants._SendfileMode.NATIVE: |
Lib/asyncio/base_events.py
Outdated
| except events.SendfileNotAvailableError as exc: | ||
| if not fallback: | ||
| raise | ||
| # the mode is FALLBACK or fallback is True |
Lib/asyncio/base_events.py
Outdated
| break | ||
| fut = proto._paused | ||
| if fut is not None: | ||
| if await fut: |
There was a problem hiding this comment.
Let's use another enum for the values of proto._paused future; that will improve the readability a great deal.
|
|
||
| def __init__(self, loop, sock, protocol, waiter=None, | ||
| extra=None, server=None): | ||
| super().__init__(loop, sock, protocol, extra, server) | ||
| self._eof = False | ||
| self._paused = False | ||
| self._empty_waiter = None |
There was a problem hiding this comment.
_empty_waiter should handle a case when it exists and a connection is aborted/closed. In which case we want to cancel it.
There was a problem hiding this comment.
Cancelling is confusing.
I prefer another exception -- and OSError or derived type is explicitly set
There was a problem hiding this comment.
Whatever, just make sure that if something awaits on _empty_writer does not wait forever because we don't cancel the future.
Lib/asyncio/base_events.py
Outdated
|
|
||
| def connection_made(self, transport): | ||
| raise RuntimeError("Invalid state, " | ||
| "connection should be established already.") |
Lib/asyncio/base_events.py
Outdated
|
|
||
| class _SendfileProtocol(protocols.Protocol): | ||
| def __init__(self, transp): | ||
| # transport should be _FlowControlMixin instance |
There was a problem hiding this comment.
Can you add an if not isinstance(transp, _FlowControlMixin): raise TypeError. I know that it shouldn't be possible to get this exception, but in case we made a mistake and some user hits this error message, it would be more informative than some random AttributeError.
There was a problem hiding this comment.
Perform the check in debug mode only maybe?
There was a problem hiding this comment.
sendfile isn't a frequent operation so one isinstance call won't slow it down, so let's check it always.
Lib/asyncio/selector_events.py
Outdated
| @@ -765,6 +782,9 @@ def write(self, data): | |||
| f'not {type(data).__name__!r}') | |||
| if self._eof: | |||
| raise RuntimeError('Cannot call write() after write_eof()') | |||
| if self._empty_waiter is not None: | |||
| raise RuntimeError('Cannot call write() when loop.sendfile() ' | |||
| 'is not finished') | |||
There was a problem hiding this comment.
unable to write; sendfile is in progress
Lib/asyncio/base_events.py
Outdated
| """Send a file through a transport. | ||
|
|
||
| Return amount of sent bytes. | ||
| """ |
There was a problem hiding this comment.
Copy the relevant snippet from the documentation to make it a proper docstring.
There was a problem hiding this comment.
Did it but I feel that docstring is too long.
Lib/asyncio/selector_events.py
Outdated
| transp.pause_reading() | ||
| fut = transp._make_empty_waiter() | ||
| await fut | ||
| try: |
There was a problem hiding this comment.
await transp._make_empty_waiter() seems to be a bit shorter/readable.
|
Overall LGTM, left a couple minor comments. |
|
@asvetlov: Please replace |
|
Uhhh. Done, finally. |
|
Thank you, Andrew! I'm super happy about asyncio finally getting first class sendfile support! |
|
It means I can drop ugly sendfile hacks from aiohttp :) |
https://bugs.python.org/issue32622