bpo-30844: selectors: add urgent data to read event #2562
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. Thanks again to your contribution and we look forward to looking at it! |
vstinner
left a comment
There was a problem hiding this comment.
Wow, nice patch! Todo:
- Add a NEWS entry using the blurb tool, see: https://docs.python.org/devguide/committing.html#what-s-new-and-news-entries
- Document the 2 newly added methods in Doc/library/selectors.rst, don't forget ".. versionadded:: 3.7"
- Maybe also document the enhancement in Doc/whatsnew/3.7.rst, in a new "selectors" section
vstinner
left a comment
There was a problem hiding this comment.
I would prefer to first focus on the selectors module to have a shorter PR, easier to review. And later extend asyncio.
I mean, keep your asyncio changes somewhere, and revert all asyncio changes here.
|
If I understand correctly, you want me to:
Step 2 and 3 can not exist without this patch. Chapter 11 of the Python Developers Guide, Triaging an issue states under 11.1.10. Dependencies, that dependencies can be listed to be resolved first, before an issue can move forward. Since their is no field marked "dependencies" when creating a new issue, do I just list them in the comment? |
|
"create a second patch solving the second issue" This can wait until this PR is merged ;-) |
|
I am sorry, I see now that I have committed a foul (no pun intended). I removed the commit regarding asyncio from my branch and executed a force push to GitHub, something that is clearly listed as not allowed (sorry, newby). Do I need to fix this by reverting the change in the title, adding the commit again and then remove it explicitly and then again change the title, or should I just leave it for now |
|
You can use git push --force on your branch, pklanke:fix-issue-30844. I'm doing that multiple times per day :-) When I'm in my local branch (ex: "fix-issue-30844" in your case), I use "git push haypo HEAD -f", where "haypo" is my Git remote repostory (see "git remote -v"). |
|
As a side note, |
|
"As a side note, kqueue does not seem to have support for urgent data. Asking for urgent data with kqueue should probably fail with a well-defined error (NotImplementedError?)." Oh. Maybe we should not add add_excepter() on KqueueSelector? |
|
"Oh. Maybe we should not add add_excepter() on KqueueSelector?" You're confusing me; You asked me to remove the asyncio related part from this patch and solely focus on the selectors patch first. add_excepter() is part of the asyncio patch and no longer a part of this patch. On top of that, add_excepter is, was, and will not be added to KqueueSelector. |
|
"You're confusing me; You asked me to remove the asyncio related part from this patch and solely focus on the selectors patch first. add_excepter() is part of the asyncio patch and no longer a part of this patch." Sorry, I didn't read carefully your PR. You're right, I'm wrong :-) Ignore my previous comment. Sadly, it seems like NotImplementedError is the only option for KqueueSelector. It should be carefully documented. |
|
The kernel documentations for sysfs states the following:
In my opinion, sysfs kind of misuses available events to be able to support edge triggered io events, since the read event can not be used (sysfs GPIO fd's are always available for read). However, the patch has no relation to sysfs or TCP, sysfs is only mentioned as a use case for the exceptional conditions event. Naming the event is not based on what it is used for, but on what lies beneath: man select(2), select_tot(2), poll(2) and epoll(7). IMO 'exceptional conditions event' is the best choice here, since this is the common denominator used by select (which was what we used before the selectors module was added in python 3.4). The term 'urgent data' originates from (e)poll, which some might say could also be used, but this would result in adding add_urgenter() to asyncio's selector_events module (again, not part of this patch anymore) I really regret using the term 'urgent data' in the documentation and the title now ;-). Can we agree on calling it 'exceptional condition'? |
|
Please move the discussion to http://bugs.python.org/issue30844 (we prefer
discusson on bugs.python.org rather than GitHub, and I would prefer to not
discuss at two different places), thanks ;-)
|
d0415dd to
441380f
Compare
b2a369d to
8f31d90
Compare
Lib/selectors.py
Outdated
There was a problem hiding this comment.
It would be nice to add a comment explaining the w|x thing as well as the _w+_x below.
8f31d90 to
12b3764
Compare
|
All checks have passed and no conflicts exist with the base branch. Is there anything more I can or need to to do to get this merged into the master branch? |
Signed-off-by: Pim Klanke <pim@protonic.nl>
Signed-off-by: Pim Klanke <pim@protonic.nl>
This fixes an inconsistency between base class' select method, which uses predefined class members _EVENT_READ and _EVENT_WRITE, and EpollSelector.select, which uses select.EPOLL* events directly. Signed-off-by: Pim Klanke <pim@protonic.nl>
Signed-off-by: Pim Klanke <pim@protonic.nl>
Naming the event for the exceptional conditions event was mainly based on exceptfds, the 3rd argument of select(). Since except already has a very distinctive meaning in the world of Python, it is better to replace it by one of the already accepted synonyms, in this case "urgent data to read" (used by poll()). Signed-off-by: Pim Klanke <pim@protonic.nl>
Signed-off-by: Pim Klanke <pim@protonic.nl>
Signed-off-by: Pim Klanke <pim@protonic.nl>
precomputing the masks moves unnecessary computation out of the loop. Signed-off-by: Pim Klanke <pim@protonic.nl>
12b3764 to
3c60361
Compare
|
This PR is stale because it has been open for 30 days with no activity. |
MaxwellDupre
left a comment
There was a problem hiding this comment.
Got an error when running specific test:
./python -m unittest -v test.test_selectors.test_main
Ran 114 tests in 18.104s
OK (skipped=40)
Traceback (most recent call last):
File "/home/me/Documents/cpython/Lib/runpy.py", line 193, in _run_module_as_main
"main", mod_spec)
File "/home/me/Documents/cpython/Lib/runpy.py", line 85, in _run_code
exec(code, run_globals)
File "/home/me/Documents/cpython/Lib/unittest/main.py", line 18, in
main(module=None)
File "/home/me/Documents/cpython/Lib/unittest/main.py", line 94, in init
self.parseArgs(argv)
File "/home/me/Documents/cpython/Lib/unittest/main.py", line 141, in parseArgs
self.createTests()
File "/home/me/Documents/cpython/Lib/unittest/main.py", line 148, in createTests
self.module)
File "/home/me/Documents/cpython/Lib/unittest/loader.py", line 219, in loadTestsFromNames
suites = [self.loadTestsFromName(name, module) for name in names]
File "/home/me/Documents/cpython/Lib/unittest/loader.py", line 219, in
suites = [self.loadTestsFromName(name, module) for name in names]
File "/home/me/Documents/cpython/Lib/unittest/loader.py", line 211, in loadTestsFromName
(obj, test))
TypeError: calling <function test_main at 0x7ff7a4200338> returned None, not a test
cpython on fix-issue-30844 [$?] via 🐍 v3.11.0a3+ took 18s
|
This PR is stale because it has been open for 30 days with no activity. |
|
This PR is stale because it has been open for 30 days with no activity. |
https://bugs.python.org/issue30844