bpo-32622: Implement loop.sendfile() by asvetlov · Pull Request #5271 · python/cpython

bpo-32622: Implement loop.sendfile() by asvetlov · Pull Request #5271 · python/cpython


		def connection_made(self, transport):
		raise RuntimeError("Broken state, "
		"connection should be established already.")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Invalid state: connection should have been established already"

		self._paused = None

		def data_received(self, data):
		raise RuntimeError("Broken state, reading should be paused")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

		self._transport = transp
		self._proto = transp.get_protocol()
		self._resume_reading = transp.is_reading()
		self._write_paused = transp._protocol_paused

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd call these '_should_resume_reading' and '_should_resume_writing'

		if mode is constants._SendfileMode.UNSUPPORTED:
		raise RuntimeError(
		f"sendfile is not supported for transport {transport!r}")
		if mode is constants._SendfileMode.NATIVE:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd call it TRY_NATIVE

		except events.SendfileNotAvailableError as exc:
		if not fallback:
		raise
		# the mode is FALLBACK or fallback is True

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd put an assert check here.

		break
		fut = proto._paused
		if fut is not None:
		if await fut:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use another enum for the values of proto._paused future; that will improve the readability a great deal.

		super().__init__(loop, sock, protocol, extra, server)
		self._eof = False
		self._paused = False
		self._empty_waiter = None

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_empty_waiter should handle a case when it exists and a connection is aborted/closed. In which case we want to cancel it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case is still isn't handled.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cancelling is confusing.
I prefer another exception -- and OSError or derived type is explicitly set

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whatever, just make sure that if something awaits on _empty_writer does not wait forever because we don't cancel the future.


		def connection_made(self, transport):
		raise RuntimeError("Invalid state, "
		"connection should be established already.")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"should be" -> "should have been"


		class _SendfileProtocol(protocols.Protocol):
		def __init__(self, transp):
		# transport should be _FlowControlMixin instance

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perform the check in debug mode only maybe?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sendfile isn't a frequent operation so one isinstance call won't slow it down, so let's check it always.

		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')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unable to write; sendfile is in progress

		"""Send a file through a transport.

		Return amount of sent bytes.
		"""

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy the relevant snippet from the documentation to make it a proper docstring.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did it but I feel that docstring is too long.

		transp.pause_reading()
		fut = transp._make_empty_waiter()
		await fut
		try:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

await transp._make_empty_waiter() seems to be a bit shorter/readable.

Overall LGTM, left a couple minor comments.

1st1 approved these changes Jan 26, 2018

@asvetlov: Please replace # with GH- in the commit message next time. Thanks!

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 :)