Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.1k
bpo-40007: Make asyncio.transport.writelines on selector use sendmsg#19062
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
Thanks for the PR@tzickel.
Assuming the general idea is approved of by Yury and/or Andrew, this PR will likely need to include a unit test that ensures it behaves as expected; it would be best included inLib/test/test_asyncio/test_selector_events.py
. See the existing transport tests in there for examples, such as the ones that start withtest_write*
.
The PR could also use a Misc/NEWS entry to briefly explain the changes.
In the meantime, I have a few suggestions/general comments:
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
aeros commentedMar 19, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Also, the current CI failures will have to be addressed of course. My comments above were just concerns that I noticed at a glance. I suspect that some of the failures may have been occurring as a result of the |
@aeros Thanks for your comments, I've hopefully fixed the code / logic mistakes. |
I wasn't referring to repeating the Current: sendmsg=getattr(socket.socket,"sendmsg",False)# [snip]defwritelines(self,lines):ifnotsendmsg:returnself.write(b''.join(lines))# [snip] Recommended: # Used to determine if platform supports sendmsg to sockets# [_NAME is our typical convention for internal globals, to differentiate it]_SENDMSG=Noneclass_SelectorSocketTransport(_SelectorTransport):# [snip]def__init__(self,loop,sock,protocol,waiter=None,extra=None,server=None):global_SENDMSG# [ensures the `getattr()` is only done once, but doesn't create extra# overhead when select_events is imported]if_SENDMSGisNone:_SENDMSG=getattr(socket.socket,"sendmsg",False)self._sendmsg=_SENDMSG# [snip]defwritelines(self,lines):ifnotself._sendmsg:returnself.write(b''.join(lines))# [snip] Does that make it more clear? |
aeros commentedMar 21, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Also, as a side note, there's typically no need to force push over old commits for PR branches in the CPython repo since we squash prior to merging. It canoccasionally help if it the history becomes muddled over time, but it can also make changes made to the PR harder to follow for review purposes. Seehttps://discuss.python.org/t/pep-601-forbid-return-break-continue-breaking-out-of-finally/2239/42. It's also mentioned at the end ofhttps://devguide.python.org/pullrequest/#submitting. |
#31871 is the newer resurrection of the idea. |
Uh oh!
There was an error while loading.Please reload this page.
https://bugs.python.org/issue40007