GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-27456: Ensure TCP_NODELAY is set on linux by 1st1 · Pull Request #4231 · python/cpython

Conversation

@1st1

@1st1 1st1 commented Nov 2, 2017 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

https://bugs.python.org/issue27456

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@1st1 1st1 added the needs backport to 3.6 label Nov 2, 2017
@1st1 1st1 self-assigned this Nov 2, 2017
@1st1
1st1 requested review from asvetlov and vstinner November 2, 2017 15:57
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Nov 2, 2017
@bedevere-bot bedevere-bot added the awaiting merge label Nov 2, 2017
@1st1 1st1 mentioned this pull request Nov 2, 2017
bpo-27456: asyncio: Fix _set_nodelay() for non-blocking socket #4233
Closed
vstinner
vstinner reviewed Nov 2, 2017
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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.

Please copy/paste my unit test into your PR: https://github.com/python/cpython/pull/4233/files

You may also copy my NEWS entry. IMHO it's worth it to document this bugfix.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Mariatta
Mariatta reviewed Nov 2, 2017
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2017-11-02-11-57-41.bpo-27456.snzyTC.rst Outdated

@Mariatta Mariatta Nov 2, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Please end the sentence with a period. :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@1st1 1st1 Nov 2, 2017

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Mariatta reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
vstinner
vstinner approved these changes Nov 2, 2017
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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 didn't check that you replaced all sock with sock.type, but tests pass so it should be fine :-)

LGTM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner

vstinner commented Nov 2, 2017

Copy link
Copy Markdown
Member

Oops, no, Travis CI failed.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@asvetlov

asvetlov commented Nov 3, 2017

Copy link
Copy Markdown
Contributor

I don't figure out why the failed test shouldn't go though fastpath
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner requested changes Nov 3, 2017
View reviewed changes
Comment thread Lib/asyncio/base_events.py Outdated

@vstinner vstinner Nov 3, 2017

Copy link
Copy Markdown
Member

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 change is ok. But please fix also "type == socket.SOCK_DGRAM" below, fix test_ipaddr_info(), and please also write a test for non-blocking DGRAM socket.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Nov 6, 2017

Copy link
Copy Markdown
Member

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 @1st1

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot removed the awaiting merge label Nov 3, 2017
@bedevere-bot

bedevere-bot commented Nov 3, 2017

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting changes label Nov 3, 2017
@1st1

1st1 commented Nov 6, 2017

Copy link
Copy Markdown
Member Author

I'll work on this later today.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vmagamedov
vmagamedov reviewed Nov 7, 2017
View reviewed changes
Comment thread Lib/asyncio/base_events.py Outdated

@vmagamedov vmagamedov Nov 7, 2017

Copy link
Copy Markdown

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.

socket.SOCK_STREAM is not a bit, it's a value, but socket.SOCK_NONBLOCK is a bit. That's why it is weird and complicated. Values and socket creation flags are mixed. For example, this check will fail for sock_type == socket.SOCK_SEQPACKET:

>>> (socket.SOCK_SEQPACKET & socket.SOCK_STREAM) == socket.SOCK_STREAM
True

All possible values on Linux:

>>> list(socket.SocketKind)
[<SocketKind.SOCK_STREAM: 1>,
 <SocketKind.SOCK_DGRAM: 2>,
 <SocketKind.SOCK_RAW: 3>,
 <SocketKind.SOCK_RDM: 4>,
 <SocketKind.SOCK_SEQPACKET: 5>,
 <SocketKind.SOCK_NONBLOCK: 2048>,
 <SocketKind.SOCK_CLOEXEC: 524288>]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
pitrou
pitrou reviewed Nov 16, 2017
View reviewed changes
Comment thread Lib/asyncio/selector_events.py Outdated

@pitrou pitrou Nov 16, 2017

Copy link
Copy Markdown
Member

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.

At this point, how about switching to EAFP (Easier to Ask Forgiveness than Permission)?

try:
    sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1)
except OSError:
    pass

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@1st1 1st1 Nov 16, 2017

Copy link
Copy Markdown
Member Author

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.

Yeah, that's what I'm planning to do here. There appears to be no reliable and future-proof way of checking socket type on Linux, so EAFP is the best option. The "not so good" option is to do a dance like this:

if sock.type & ~(SOCK_NONBLOCK | SOCK_CLOEXEC) == SOCK_STREAM:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Nov 17, 2017

Copy link
Copy Markdown
Member

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 prefer to filter flags rather than always calling setsockopt() but ignore errors. For best performance, I prefer to avoid useless syscalls.

sock.type & ~(SOCK_NONBLOCK | SOCK_CLOEXEC)

Maybe we should add an attribute to socket.socket which returns the type without SOCK_NONBLOCK and SOCK_CLOEXEC bits.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
asvetlov reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@vmagamedov vmagamedov Nov 17, 2017

Copy link
Copy Markdown

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.

Another option is to add SOCK_TYPE_MASK, maybe even as socket.SOCK_TYPE_MASK, as Linux does (https://github.com/torvalds/linux/blob/v4.13/include/linux/net.h#L77).

I think that

sock.type & ~(SOCK_NONBLOCK | SOCK_CLOEXEC)

is not as future-proof as

sock.type & SOCK_TYPE_MASK

Shouldn't BSD/Linux devs support backward-compatibility/conventions after introducing SOCK_CLOEXEC and SOCK_NONBLOCK flags? Maybe using SOCK_TYPE_MASK will be safe enough?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@1st1 1st1 Nov 17, 2017

Copy link
Copy Markdown
Member Author

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 like SOCK_TYPE_MASK idea. At least we can do this on Linux.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@1st1 1st1 Nov 17, 2017

Copy link
Copy Markdown
Member Author

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 do some experiments/more digging and update this PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pitrou pitrou Nov 17, 2017

Copy link
Copy Markdown
Member

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.

For best performance, I prefer to avoid useless syscalls.

At the cost of maintainability, for a syscall that's called once in a socket's lifetime...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Nov 17, 2017

Copy link
Copy Markdown
Member

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.

About being "future-proof": SOCK_NONBLOCK and SOCK_CLOEXEC flags are now old and well known. But I agree that it's annoying to have to care of them :-(

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pitrou pitrou Nov 17, 2017

Copy link
Copy Markdown
Member

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. 

>>> def f(sock):
...:    try:
...:        sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1)
...:    except OSError:
...:        pass
...:    
>>> sock = socket.socket(socket.AF_INET)
>>> %timeit f(sock)
500 ns ± 18.3 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
>>> sock = socket.socket(socket.AF_UNIX)
>>> %timeit f(sock)
1.66 µs ± 6.82 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner

vstinner commented Nov 17, 2017

Copy link
Copy Markdown
Member

While the idea of a new socket.socket attribute getting type without flags and SOCK_TYPE_MASK idea are nice, I suggest to keep this PR simple as the initially proposed fix, and open a new issue on bugs.python.org to propose to enhance the socket module.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@asvetlov

asvetlov commented Dec 13, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

ping
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Yury Selivanov
bpo-27456: Ensure TCP_NODELAY is set on linux
2e208b5
@1st1
1st1 force-pushed the fixaio branch from 2380a91 to 2e208b5 Compare December 14, 2017 04:25
@1st1

1st1 commented Dec 14, 2017

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Dec 14, 2017

Copy link
Copy Markdown

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Dec 14, 2017
@1st1

1st1 commented Dec 14, 2017

Copy link
Copy Markdown
Member Author

@vstinner I think this should now be ready to be merged.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
asvetlov
asvetlov approved these changes Dec 14, 2017
View reviewed changes
@bedevere-bot bedevere-bot removed the awaiting change review label Dec 14, 2017
@bedevere-bot bedevere-bot added the awaiting merge label Dec 14, 2017
vstinner
vstinner previously requested changes Dec 14, 2017
View reviewed changes
Comment thread Lib/asyncio/base_events.py
# `sock_type == socket.SOCK_STREAM`, see
# https://github.com/torvalds/linux/blob/v4.13/include/linux/net.h#L77
# for more details.
return (sock_type & 0xF) == socket.SOCK_STREAM

@vstinner vstinner Dec 14, 2017

Copy link
Copy Markdown
Member

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.

Would it be possible to add a private constant to the socket module, rather rhan using a private constant? If available, even use the Linux constant.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Dec 14, 2017

Copy link
Copy Markdown
Member

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 constant can be ~0 if SOCK_NONBLOCK is missing to prevent the if.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@1st1 1st1 Dec 15, 2017

Copy link
Copy Markdown
Member Author

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.

Would it be possible to add a private constant to the socket module, rather rhan using a private constant?

Maybe, but I'd like to do that in a separate PR to the socket module first. I'd like to merge this PR as is.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@1st1 1st1 Dec 15, 2017

Copy link
Copy Markdown
Member Author

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.

IOW -- in my opinion this PR should be merged as is and backported to 3.6. I'll work on a new PR to expose SOCK_TYPE_MASK in socket and we'll update asyncio to use that.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@asvetlov asvetlov Dec 15, 2017

Copy link
Copy Markdown
Contributor

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 ok with merging, backporting to 3.6 and polishing 3.7 at the end.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot

bedevere-bot commented Dec 14, 2017

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting merge labels Dec 14, 2017
Yury Selivanov
Remove nonsense test
3c0f674
@vstinner
vstinner dismissed their stale review December 16, 2017 00:30

A more generic fix is in progress: https://bugs.python.org/issue32331

@1st1

1st1 commented Dec 16, 2017

Copy link
Copy Markdown
Member Author

A more generic fix is in progress: https://bugs.python.org/issue32331

Yes, I'll merge this in now, and when that issue is resolved, I'll update the code for 3.7.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@1st1
1st1 merged commit e796b2f into python:master Dec 16, 2017
@miss-islington

miss-islington commented Dec 16, 2017

Copy link
Copy Markdown
Contributor

Thanks @1st1 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting changes label Dec 16, 2017
@miss-islington

miss-islington commented Dec 16, 2017

Copy link
Copy Markdown
Contributor

Sorry, @1st1, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker e796b2fe26f220107ac50667de6cc86c82b465e3 3.6
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
1st1 pushed a commit to 1st1/cpython that referenced this pull request Dec 16, 2017
Yury Selivanov
bpo-27456: Ensure TCP_NODELAY is set on linux (python#4231)
6547094 
(cherry picked from commit e796b2f)
@1st1

1st1 commented Dec 16, 2017

Copy link
Copy Markdown
Member Author

Backport PR: #4898
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
1st1 pushed a commit that referenced this pull request Dec 16, 2017
Yury Selivanov
bpo-27456: Ensure TCP_NODELAY is set on linux (#4231) (#4898)
572636d 
(cherry picked from commit e796b2f)
@bedevere-bot

bedevere-bot commented Mar 18, 2018

Copy link
Copy Markdown

GH-4898 is a backport of this pull request to the 3.6 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.6 label Mar 18, 2018
@tjhowse tjhowse mentioned this pull request Feb 2, 2024
create_datagram_endpoint() no longer compatible with CAN bus raw sockets #114887
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asvetlov asvetlov asvetlov approved these changes
@pitrou pitrou pitrou left review comments
@Mariatta Mariatta Mariatta left review comments
@vstinner vstinner vstinner left review comments
+1 more reviewer
@vmagamedov vmagamedov vmagamedov left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@1st1 1st1

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@1st1 @vstinner @asvetlov @bedevere-bot @miss-islington @vmagamedov @pitrou @Mariatta @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.