Skip to content

bpo-40007: Make asyncio.transport.writelines on selector use sendmsg - #19062

Closed
tzickel wants to merge 1 commit into
python:mainfrom
tzickel:asynciowritev
Closed

bpo-40007: Make asyncio.transport.writelines on selector use sendmsg#19062
tzickel wants to merge 1 commit into
python:mainfrom
tzickel:asynciowritev

Conversation

@tzickel

@tzickeltzickel commented Mar 18, 2020

Copy link
Copy Markdown
Contributor

@tzickeltzickel changed the title bpo-XXX: Make asyncio.transport.writelines on selector use sendmsgbpo-40007: Make asyncio.transport.writelines on selector use sendmsgMar 18, 2020

@aerosaeros left a comment

Copy link
Copy Markdown
Contributor

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 in Lib/test/test_asyncio/test_selector_events.py. See the existing transport tests in there for examples, such as the ones that start with test_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:

Comment threadLib/asyncio/selector_events.py Outdated
Comment threadLib/asyncio/selector_events.py Outdated
Comment threadLib/asyncio/selector_events.py Outdated
Comment threadLib/asyncio/selector_events.py Outdated
Comment threadLib/asyncio/selector_events.py Outdated
@aeros

aeros commented Mar 19, 2020

Copy link
Copy Markdown
Contributor

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 seld._buffer code typos that I mentioned, but I haven't looked over them yet.

@tzickel

Copy link
Copy Markdown
ContributorAuthor

@aeros Thanks for your comments, I've hopefully fixed the code / logic mistakes.

@aeros

Copy link
Copy Markdown
Contributor

@tzickel

Thanks, but it's a small one-time overhead instead of a small overhead on each object initialization

I wasn't referring to repeating the getattr(socket.socket, "sendmsg", False) every time the object is created; that's not necessary. Here's an example of what I had in mind:

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

aeros commented Mar 21, 2020

Copy link
Copy Markdown
Contributor

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 can occasionally 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.

See https://discuss.python.org/t/pep-601-forbid-return-break-continue-breaking-out-of-finally/2239/42. It's also mentioned at the end of https://devguide.python.org/pullrequest/#submitting.

@asvetlov

Copy link
Copy Markdown
Contributor

#31871 is the newer resurrection of the idea.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@tzickel@aeros@asvetlov@the-knights-who-say-ni@bedevere-bot