Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Closed
tzickel wants to merge1 commit intopython:mainfromtzickel:asynciowritev

Conversation

tzickel
Copy link
Contributor

@tzickeltzickel commentedMar 18, 2020
edited by bedevere-bot
Loading

@tzickeltzickel changed the titlebpo-XXX: Make asyncio.transport.writelines on selector use sendmsgbpo-40007: Make asyncio.transport.writelines on selector use sendmsgMar 18, 2020
Copy link
Contributor

@aerosaeros left a comment
edited
Loading

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:

@aeros
Copy link
Contributor

aeros commentedMar 19, 2020
edited
Loading

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

@tzickel
Copy link
ContributorAuthor

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

@aeros
Copy link
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 thegetattr(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
Copy link
Contributor

aeros commentedMar 21, 2020
edited
Loading

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.

@asvetlov
Copy link
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
Reviewers

@aerosaerosaeros left review comments

@1st11st1Awaiting requested review from 1st11st1 is a code owner

@asvetlovasvetlovAwaiting requested review from asvetlovasvetlov is a code owner

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

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

[8]ページ先頭

©2009-2025 Movatter.jp