Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-32568: make select.epoll() and its docs consistent #7840

Merged
merged 5 commits into from Jun 30, 2018

Conversation

taleinat
Copy link
Contributor

@taleinat taleinat commented Jun 21, 2018

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)
Copy link
Member

@vstinner vstinner left a comment

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*
Copy link
Member

@vstinner vstinner Jun 22, 2018

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".

@bedevere-bot
Copy link

bedevere-bot commented Jun 22, 2018

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

@taleinat
Copy link
Contributor Author

taleinat commented Jun 22, 2018

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.

@taleinat
Copy link
Contributor Author

taleinat commented Jun 22, 2018

I have made the requested changes; please review again.

@bedevere-bot
Copy link

bedevere-bot commented Jun 22, 2018

Thanks for making the requested changes!

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

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Jun 25, 2018

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.

@taleinat
Copy link
Contributor Author

taleinat commented Jun 25, 2018

@serhiy-storchaka, this is ready for another review.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Thanks @taleinat!

@taleinat taleinat merged commit 0cdf5f4 into python:master Jun 30, 2018
@bedevere-bot
Copy link

bedevere-bot commented Jun 30, 2018

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

@miss-islington
Copy link
Contributor

miss-islington commented Jun 30, 2018

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

@bedevere-bot
Copy link

bedevere-bot commented Jun 30, 2018

GH-8024 is a backport of this pull request to the 3.7 branch.

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>
@miss-islington
Copy link
Contributor

miss-islington commented Jun 30, 2018

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>
@bedevere-bot
Copy link

bedevere-bot commented Jun 30, 2018

GH-8025 is a backport of this pull request to the 3.6 branch.

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)
yahya-abou-imran pushed a commit to yahya-abou-imran/cpython that referenced this pull request Nov 2, 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants