Skip to content

gh-79698: selector.EpollSelector: add new parameter to support extra events #11193

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

gh-79698: selector.EpollSelector: add new parameter to support extra events #11193

wants to merge 1 commit into from

Conversation

Zheaoli
Copy link
Contributor

@Zheaoli Zheaoli commented Dec 17, 2018 •
edited by bedevere-app bot
Loading

asvetlov
asvetlov reviewed Dec 17, 2018
View reviewed changes
Copy link
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

A documentation update is needed

Lib/selectors.py Outdated
@@ -348,18 +348,15 @@ def __init__(self):
super().__init__()
self._selector = self._selector_cls()

def register(self, fileobj, events, data=None, *, exclusive=False):
def register(self, fileobj, events, data=None, *, extra_events=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

extra_events=0 maybe?

Copy link
Member

Choose a reason for hiding this comment

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

I prefer None, but no False plz.

Copy link
Contributor

Choose a reason for hiding this comment

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

None?

Copy link
Contributor

Choose a reason for hiding this comment

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

extra_events is a bitmask, why None?

Copy link
Contributor Author

@Zheaoli Zheaoli Dec 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

extra_events is a bitmask, why None?

In the most circumstance, people do not need the extra_events , so I give it a default value

@asvetlov

Copy link
Contributor

Choose a reason for hiding this comment

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

With 0 default value people still don't need to pass extra_events.
But we don't need to extra check the value for None, 0 should work fine as no flags for all possible selectors: poll, epoll, kqueue etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that sounds a good idea. I will fix it! Thx!

@asvetlov
Copy link
Contributor

Should modify() method be updated as well?
Should new argument be mentioned in the base abstract class?

@zhangyangyu
Copy link
Member

zhangyangyu commented Dec 17, 2018 •
edited
Loading

After this change, we could pass more eventmask in, even wrong ones (not before since only read/write allowed). What to do with them?

@Zheaoli
Copy link
Contributor Author

Zheaoli commented Dec 17, 2018

@zhangyangyu

In select module, people use it by passing event mask directly.
So, I think it's necessary to check the extra_events.

@Zheaoli Zheaoli force-pushed the bpo-35517 branch 2 times, most recently from a897190 to 234f30e Compare December 17, 2018 15:47
@asvetlov
Copy link
Contributor

Regarding to check for invalid event mask: the underlying call to epoll (or another poller API) should raise an exception for invalid mask bits.
Why check them again in selectors?

@Zheaoli Zheaoli changed the title bpo-35517: selector.EpollSelector: add new parameter to support EPOLLEXCLUSIVE bpo-35517: selector.EpollSelector: add new parameter to support extra events Dec 17, 2018
@giampaolo
Copy link
Contributor

I commented on the bug tracker.

@zhangyangyu
Copy link
Member

zhangyangyu commented Dec 18, 2018 •
edited
Loading

Why check them again in selectors?

@asvetlov It's the current status of register and modify, see their docs:

This returns a new SelectorKey instance, or raises a ValueError in case of invalid event mask or file descriptor, or KeyError if the file object is already registered.

And IMHO, a ValueError is easier to identify the root cause of the error than an OSError of poll. And as my test and understanding the manual, both poll and epoll_wait won't return error for invalid bitmask, it seems just pick the bitmask it thinks valid.

But it seems it's somewhat messy to implement the check. Relying on the users to always use constants in select module is also acceptable.

@dgilman dgilman mentioned this pull request Aug 19, 2021
Open
MaxwellDupre

This comment was marked as outdated.

Sign in to view

@erlend-aasland erlend-aasland changed the title bpo-35517: selector.EpollSelector: add new parameter to support extra events gh-79698: selector.EpollSelector: add new parameter to support extra events Nov 8, 2024
@erlend-aasland
Copy link
Contributor

@Zheaoli, would you mind to resolve the conflicts and pull in main?

@github-actions GitHub Actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Apr 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asvetlov asvetlov asvetlov left review comments

@zhangyangyu zhangyangyu zhangyangyu left review comments

@eamanu eamanu eamanu left review comments

@MaxwellDupre MaxwellDupre MaxwellDupre approved these changes

@giampaolo giampaolo Awaiting requested review from giampaolo

Assignees
No one assigned
Labels
awaiting core review stale Stale PR or inactive for long period of time.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@Zheaoli @asvetlov @zhangyangyu @giampaolo @erlend-aasland @eamanu @MaxwellDupre @the-knights-who-say-ni @ezio-melotti @bedevere-bot