Skip to content

Conversation

@asvetlov
Copy link
Contributor

@asvetlov asvetlov commented Feb 6, 2018

Implement native sendfile on proactor event loop

https://bugs.python.org/issue32622

@asvetlov
Copy link
Contributor Author

@1st1 please review

Copy link
Member

@1st1 1st1 left a comment

Choose a reason for hiding this comment

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

Overall looks good.


def _force_close(self, exc):
if self._empty_waiter is not None:
self._empty_waiter.set_exception(exc)
Copy link
Member

Choose a reason for hiding this comment

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

exc can be None, for instance when transport.abort() is called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch

self.run_loop(
self.loop.sendfile(cli_proto.transport, self.file,
fallback=False))
fallback=False))
Copy link
Member

Choose a reason for hiding this comment

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

This looks like an unrelated change that will cause a linting error, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


@unittest.skipIf(sys.platform != 'win32',
'Proactor is supported on Windows only')
class ProactorEventLoopUnixSockSendfileTests(test_utils.TestCase):
Copy link
Member

Choose a reason for hiding this comment

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

Are these entirely new tests? I thought we already had many functional tests for *nix. Can we just reuse those?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was my try but I figured out that tests from test_unix_events_py not completely functional only, they use selector specific mocks for testing edge cases.
Proactor based implementation is different.

Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to have a common base class with functional tests and subclasses for selector- and proactor-loops with custom setUp() methods for mocking?

Having one source for sendfile tests would be super great and useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try to master something

Copy link
Member

Choose a reason for hiding this comment

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

ping

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm on conference now, will have a time to work on tests at the beginning of the next week.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@asvetlov
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@1st1: please review the changes made to this pull request.

@1st1
Copy link
Member

1st1 commented Feb 21, 2018

Andrew, can we move all sendfile-related tests to a new test_sendfile.py file? I'm getting tired of multi-thousand-lines long test_events.py and test_base_events.py.

Copy link
Member

@1st1 1st1 left a comment

Choose a reason for hiding this comment

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

As discussed, please move the tests into a separate file in a follow up pr.

@asvetlov
Copy link
Contributor Author

We have an agreement that the test movement can be done in an separate PR.
Let's merge it as is.

@asvetlov asvetlov merged commit a19fb3c into python:master Feb 25, 2018
@bedevere-bot
Copy link

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

@miss-islington
Copy link
Contributor

Thanks @asvetlov for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 25, 2018
* Support sendfile on Windows Proactor event loop naively.
(cherry picked from commit a19fb3c)

Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>
@bedevere-bot
Copy link

GH-5890 is a backport of this pull request to the 3.7 branch.

@asvetlov asvetlov deleted the sendfile-windows branch February 25, 2018 16:36
miss-islington added a commit that referenced this pull request Feb 25, 2018
* Support sendfile on Windows Proactor event loop naively.
(cherry picked from commit a19fb3c)

Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants