bpo-32410: Implement loop.sock_sendfile() by asvetlov · Pull Request #4976 · python/cpython

asvetlov · 2017-12-22T13:08:46Z

https://bugs.python.org/issue32410

asvetlov · 2017-12-22T13:09:14Z

A draft, needs more tests and docs update.

asvetlov · 2017-12-22T13:37:38Z

@1st1 please take a look.

I've copied many code lines for pre-checks from socket.sendfile() implementation.
If you like the idea I want extract these lines from socket.sendfile into private funcs in socket module and reuse it by asyncio.

Otherwise I should duplicate not only implementation but tests too, I'd like to avoid it.

1st1 · 2017-12-23T18:34:54Z

 from . import format_helpers


+class SendfileUnsupportedError(ValueError):


SendFileNotSupported

Why not raise NotImplementedError('sendfile is not available')?

1st1 · 2017-12-23T18:36:13Z

+            os.sendfile
+        except AttributeError as exc:
+            raise events.SendfileUnsupportedError(exc)
+        self._check_sendfile_params(sock, file, offset, count)


We should support passing a FD for file.

1st1 · 2017-12-23T18:38:28Z

+        try:
+            fileno = file.fileno()
+        except (AttributeError, io.UnsupportedOperation) as exc:
+            raise events.SendfileUnsupportedError(exc)  # not a regular file


I think this is redundant. I'd do something like this:

if isinstance(file, int): fileno = file else: # factor out the 'fileno()' getting part from `_ensure_fd_no_transport`

1st1 · 2017-12-23T18:39:02Z

+        try:
+            fsize = os.fstat(fileno).st_size
+        except OSError as exc:
+            raise events.SendfileUnsupportedError(exc)  # not a regular file


raise events.SendfileUnsupportedError('fstat call failed') from exc

1st1 · 2017-12-23T18:39:35Z

Did some initial review.

1st1 · 2017-12-24T13:37:58Z

+        try:
+            os.sendfile
+        except AttributeError as exc:
+            raise NotImplementedError("os.sendfile() in not available")


Actually, I think you were right initially, Andrew, to not use the NotImplementedError. I think it means "not implemented in Python code", and shouldn't be used for signalling about missing platform capabilities. On the other hand, adding a new exception type just for sendfile doesn't feel right either. How about we simply raise a RuntimeError("os.sendfile() in not available") here, what do you think?

1st1 · 2017-12-24T13:39:03Z

+                # one being 'file' is not a regular mmap(2)-like
+                # file, in which case we'll fall back on using
+                # plain send().
+                err = NotImplementedError(exc)


I'd say raise RuntimeError('os.sendfile() call failed') from exc

1st1

Please wrap the private exception that we use to signal that there's no native sendfile into a RuntimeError. LGTM otherwise.

1st1 · 2018-01-02T11:08:20Z

+                return await self._sock_sendfile_fallback(sock, file,
+                                                          offset, count)
+            else:
+                raise


raise RuntimeError('os.sendfile is not available') from None

Proposed message text is not always correct: sometimes os.sendfile is available but passed file object cannot be used by syscall.
_SendfileNotAvailable is derived from RuntimeError now and it always contains proper message text.
The best what I can do is reraising:
raise RuntimeError(exc.args[0]) from None

In this case let's make the exception public. I'd rename it to SendfileNotAvailableError and export it to asyncio module. Don't wrap it then.

asvetlov · 2018-01-02T11:40:46Z

Two things a left:

Full coverage (need a couple mocked tests)
Unregistering fd on cancellation.

asvetlov · 2018-01-02T14:47:20Z

Done.
Everything is test covered.
Implementation raises a private _SendfileNotAvailable but it is converted into RuntimeError with proper message text when goes to user (fallback=False).

1st1

Almost there. My major question is why we need tribool for fallback? IMO it complicates the API and isnt going to be used much, doesn't it?

1st1 · 2018-01-02T17:02:36Z

+                    raise RuntimeError(exc.args[0]) from None
+
+    async def _sock_sendfile_native(self, sock, file, offset, count):
+        raise _SendfileNotAvailable("Fast sendfile is not available")


i'd change to 'sendfile syscall is not available'

or to 'os.sendfile is not available'

'fast sendfile' is rather cryptic and doesn't really tell anything to the user

1st1 · 2018-01-02T17:05:52Z

+            # EAGAIN, and I am willing to take a hit in that case in
+            # order to simplify the common case.
+            self.remove_writer(registered_fd)
+        if fut.cancelled():


do we still need this check?

1st1 · 2018-01-15T14:56:52Z

@asvetlov This PR is almost there, there are only a few nits left (and tribool commit reversal). Let's merge this in and work on sendfile for transports?

asvetlov · 2018-01-16T08:35:16Z

I hope the PR is ready now

1st1 · 2018-01-16T15:16:32Z

+        if self._debug and sock.gettimeout() != 0:
+            raise ValueError("the socket must be non-blocking")
+        self._check_sendfile_params(sock, file, offset, count)
+        if fallback:


The logic must be different.

Currently you use slow fallback code if fallback=True.

Instead you should always try to use fast syscall first, and if it fails, we check if the fallback is enabled. If it is, we emulate sendfile, if not -- we raise an error.

1st1 · 2018-01-16T15:16:50Z

I hope the PR is ready now

The fallback logic is messed up.

asvetlov · 2018-01-16T16:46:33Z

Sorry. Fixed.

1st1

I think it's in a good shape now, thanks Andrew. LGTM.

asvetlov · 2018-01-16T17:26:39Z

Thanks for careful review!

Draft for sock_sendfile

ee13d5b

asvetlov requested a review from 1st1 as a code owner December 22, 2017 13:08

the-knights-who-say-ni added the CLA signed label Dec 22, 2017

bedevere-bot added the awaiting merge label Dec 22, 2017

asvetlov added 7 commits December 22, 2017 15:38

Add test for blocked socket

8b610ad

Polish tests

8d658be

Test partial file content

24c5a28

Add NEWS entry

10b0b61

Add test for abstract sock_sendfile

5f782a5

Polishment

3068aa9

Don't call loop.set_debug(True)

898766e

asvetlov added the topic-asyncio label Dec 22, 2017

asvetlov added 4 commits December 22, 2017 17:44

Revert set_debug() back

6e70a62

Work on tests

20b3778

Improve test cleanup

7ed0e67

Make tests stable

f4b61b1

1st1 reviewed Dec 23, 2017

View reviewed changes

asvetlov added 5 commits December 24, 2017 13:26

Merge remote-tracking branch 'upstream/master' into sock_sendfile

6f6b94b

Refactor _check_sendfile_params helper

1c05795

Use NotImplementedError in private socket.sendfile implementation.

f670fad

Refactoring

26f6d4a

Polish error text

87d4804

1st1 reviewed Dec 24, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into sock_sendfile

44da800

1st1 requested changes Jan 2, 2018

View reviewed changes

Change base class for _SendfileNotAvailable to RuntimeError

967408e

asvetlov added 4 commits January 2, 2018 13:50

Better exception type when sendfile is not available

099dc56

Add a test for mixed sock_send and sock_sendfile

f9701cb

Add cancellation callback

a30acc9

More tests

f7d9bab

asvetlov added 3 commits January 2, 2018 18:04

Support tribool for fallback

4d25927

Fix signature of abstract sock_sendfile

e303db9

Merge branch 'master' into sock_sendfile

84e1057

1st1 requested changes Jan 2, 2018

View reviewed changes

Revert back tribool for fallback

657aa67

asvetlov added 3 commits January 16, 2018 10:52

Fix tests

dd4143a

Merge branch 'master' into sock_sendfile

96d0032

Add a space

5deb0e2

1st1 reviewed Jan 16, 2018

View reviewed changes

Fix sock_sendfile callback

3c9abaf

1st1 approved these changes Jan 16, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Jan 16, 2018

asvetlov merged commit 6b5a279 into python:master Jan 16, 2018

bedevere-bot removed the awaiting merge label Jan 16, 2018

asvetlov deleted the sock_sendfile branch January 16, 2018 17:59

		from . import format_helpers


		class SendfileUnsupportedError(ValueError):

Uh oh!

Conversation

asvetlov commented Dec 22, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asvetlov commented Dec 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asvetlov commented Dec 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 commented Dec 23, 2017

Uh oh!

1st1 Dec 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asvetlov commented Jan 2, 2018

Uh oh!

asvetlov commented Jan 2, 2018

Uh oh!

1st1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 commented Jan 15, 2018

Uh oh!

asvetlov commented Jan 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 commented Jan 16, 2018

Uh oh!

asvetlov commented Jan 16, 2018

Uh oh!

1st1 left a comment

Choose a reason for hiding this comment

Uh oh!

asvetlov commented Jan 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

asvetlov commented Dec 22, 2017 •

edited by bedevere-bot

Loading

asvetlov commented Dec 22, 2017 •

edited

Loading

1st1 Dec 24, 2017 •

edited

Loading