-
-
Notifications
You must be signed in to change notification settings - Fork 34k
bpo-32622: Native sendfile on windows #5565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@1st1 please review |
1st1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good.
Lib/asyncio/proactor_events.py
Outdated
|
|
||
| def _force_close(self, exc): | ||
| if self._empty_waiter is not None: | ||
| self._empty_waiter.set_exception(exc) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch
Lib/test/test_asyncio/test_events.py
Outdated
| self.run_loop( | ||
| self.loop.sendfile(cli_proto.transport, self.file, | ||
| fallback=False)) | ||
| fallback=False)) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
When you're done making the requested changes, leave the comment: |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @1st1: please review the changes made to this pull request. |
|
Andrew, can we move all sendfile-related tests to a new |
1st1
left a comment
There was a problem hiding this 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.
|
We have an agreement that the test movement can be done in an separate PR. |
|
@asvetlov: Please replace |
|
Thanks @asvetlov for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7. |
* Support sendfile on Windows Proactor event loop naively. (cherry picked from commit a19fb3c) Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>
|
GH-5890 is a backport of this pull request to the 3.7 branch. |
Implement native sendfile on proactor event loop
https://bugs.python.org/issue32622