bpo-32568: make select.epoll() and its docs consistent by taleinat · Pull Request #7840 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation14 Commits5 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

The docs for select.epoll() are currently out of sync with the implementation with regard to the sizehint and flags parameters.

flags is indeed deprecated, but there was a validation on its value, contrary to the docs saying it is "completely ignored". This removes that check.
sizehint is still used when epoll_create1() is unavailable, so this adds mention of this in the docs.
Make sizehint accept -1 again, which is replaced with FD_SETSIZE-1. This is needed to have a default value available at the Python level allowing to set this, since FD_SETSIZE is not exposed to Python. (see: bpo-31938)

https://bugs.python.org/issue32568

'flags' is indeed deprecated, but there was a validation on its value, contrary to the docs saying it is "completely ignored". This removes that check.
'sizehint' is still used when 'epoll_create1()' is unavailable, so this adds mention of this in the docs.
Make 'sizehint' accept -1 again, which is replaced with FD_SETSIZE-1. This is needed to have a default value available at the Python level allowing to set this, since FD_SETSIZE is not exposed to Python. (see: bpo-31938)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that it's correct to ignore flags. I prefer to keep the check rejecting invallid values like flags=12345.

I'm not sure why Python wants to reject unknown flags: can't we rely on the kernel/libc to validate flags?

@@ -0,0 +1 @@
make select.epoll() and its docs consistent re. sizehint and flags

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid abbreviation, english is not my first language and I don't know what "re." means. Add also an uppercase to "Make".

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

I don't think that it's correct to ignore flags. I prefer to keep the check rejecting invallid values like flags=12345.

I'm not sure why Python wants to reject unknown flags: can't we rely on the kernel/libc to validate flags?

Other than that value check, the existing code completely ignores the flag parameter. It is not used at all and specifically isn't passed to any C function. This change merely removes an unnecessary and meaningless value check.

I have made the requested changes; please review again.

Thanks for making the requested changes!

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

The argument passed to epoll_create() must be positive. Currently select.epoll() is always failed if epoll_create1() is unavailable. I think we should not accept sizehint=0.

And I think it is better to keep the check for flags. This can help to backport Python programs to older versions (the same reason as keeping the check for the argument of epoll_create() in the Linux kernel).

In future we will get rid of both parameters. Maybe will start producing a deprecation warning in 3.8.

Also update tests accordingly.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@taleinat: Please replace # with GH- in the commit message next time. Thanks!

Thanks @taleinat for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Jun 30, 2018

flags is indeed deprecated, but there is a validation on its value for backwards compatibility reasons. This adds mention of this in the docs.
The docs say that sizehint is deprecated and ignored, but it is still used when epoll_create1() is unavailable. This adds mention of this in the docs.
sizehint=-1 is acceptable again, and is replaced with FD_SETSIZE-1. This is needed to have a default value available at the Python level, since FD_SETSIZE is not exposed to Python. (see: bpo-31938)
Reject sizehint=0 since it is invalid to pass on to epoll_create().

The relevant tests have also been updated. (cherry picked from commit 0cdf5f4)

Co-authored-by: Tal Einat taleinat+github@gmail.com

Sorry, @taleinat, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 0cdf5f42898350261c5ff65d96334e736130780f 3.6

taleinat added a commit to taleinat/cpython that referenced this pull request

Jun 30, 2018

…H-7840)

flags is indeed deprecated, but there is a validation on its value for backwards compatibility reasons. This adds mention of this in the docs.
The docs say that sizehint is deprecated and ignored, but it is still used when epoll_create1() is unavailable. This adds mention of this in the docs.
sizehint=-1 is acceptable again, and is replaced with FD_SETSIZE-1. This is needed to have a default value available at the Python level, since FD_SETSIZE is not exposed to Python. (see: bpo-31938)
Reject sizehint=0 since it is invalid to pass on to epoll_create().

The relevant tests have also been updated.. (cherry picked from commit 0cdf5f4)

Co-authored-by: Tal Einat taleinat+github@gmail.com

taleinat added a commit that referenced this pull request

Jun 30, 2018

…8024)

flags is indeed deprecated, but there is a validation on its value for backwards compatibility reasons. This adds mention of this in the docs.
The docs say that sizehint is deprecated and ignored, but it is still used when epoll_create1() is unavailable. This adds mention of this in the docs.
sizehint=-1 is acceptable again, and is replaced with FD_SETSIZE-1. This is needed to have a default value available at the Python level, since FD_SETSIZE is not exposed to Python. (see: bpo-31938)
Reject sizehint=0 since it is invalid to pass on to epoll_create().

The relevant tests have also been updated.

(cherry picked from commit 0cdf5f4)

Co-authored-by: Tal Einat taleinat+github@gmail.com

taleinat added a commit that referenced this pull request

Jun 30, 2018

GH-8025)

flags is indeed deprecated, but there is a validation on its value for backwards compatibility reasons. This adds mention of this in the docs.
The docs say that sizehint is deprecated and ignored, but it is still used when epoll_create1() is unavailable. This adds mention of this in the docs.
sizehint=-1 is acceptable again, and is replaced with FD_SETSIZE-1. This is needed to have a default value available at the Python level, since FD_SETSIZE is not exposed to Python. (see: bpo-31938)
Reject sizehint=0 since it is invalid to pass on to epoll_create().

The relevant tests have also been updated.

(cherry picked from commit 0cdf5f4)