#30014: refactor poll-related classes #1035
Conversation
…poll() share the same methods for register(), unregister(), close() and select()
Lib/selectors.py
Outdated
| """Poll-based selector.""" | ||
| def __init__(self): | ||
| super().__init__() | ||
| self._poller = self._poller() |
This looks strange: I assume that self._poller is the poller class (e.g. select, poll, etc).
It's used here but not defined in this base class: maybe make it an abstract property to force its definition?
Also, assigning
self._poller = self._poller() is very confusing: you're reusing the same member for the instance and the class.
Maybe use poller_cls, or poller_type for the type of the poller, to avoid confusion with the instance?
Yes, the idea is to have a _poller attribute which can be referenced by all 3 classes. Actually I think it'd be cool to do the same also for select and kqueue.
And yes, it's probably better to also have _poller_cls.
I don't have a strong opinion about abstract properties but it's up to you.
How would you feel about having a _selector attribute for all classes (also kqueue and select)?
Sounds great.
Done
Lib/selectors.py
Outdated
| poller_events |= self._EVENT_WRITE | ||
| try: | ||
| self._poller.register(key.fd, poller_events) | ||
| except BaseException: |
I think catching BaseException is discouraged, shouldn't it be Exception?
yes, I think it should; I think I copied it from the previous code
Done.
Lib/selectors.py
Outdated
| key = super().unregister(fileobj) | ||
| try: | ||
| self._poller.unregister(key.fd) | ||
| except OSError: |
Why catch OSError here and BaseException in register()?
I copied it from the previous code:
Line 423 in 2e576f5
...although the original code did that for epoll only wheres with this it's applied to all 3 selectors. Not sure if it matters. Probably not.
In general I think it would make sense to always catch OSError (also in kqueue).
Agreed, let's catch OSError everywhere.
Lib/selectors.py
Outdated
| self._devpoll = select.devpoll() | ||
| _poller = select.devpoll | ||
| _EVENT_READ = select.POLLIN | ||
| _EVENT_WRITE = select.POLLOUT | ||
|
|
||
| def fileno(self): | ||
| return self._devpoll.fileno() |
This can't work, self._devpoll has been replaced by self._poller.
Good catch. Fixed.
Lib/selectors.py
Outdated
| ready.append((key, events & key.events)) | ||
| return ready | ||
| def close(self): | ||
| if hasattr(self._poller, "close"): |
Doing such a hasattr() is a bit naughty: why not instead override close() in EpollSelector and DevPollSelector to close the underlying FD?
If you want to maximize code reuse, maybe we could add a _FDBasedSelector - common to al implementations using a file descriptor - in which you could define close() and fileno()?
Fine with me.
Good.
I overrode close() in EpollSelector and DevPollSelector to close the underlying FD as per your suggestion.
Sorry for the delay, didn't see this message.
With the proposed changes, sounds great!
Sorry for the delay, didn't see this message.
No problem. ;-)
OK, I think it's ready.
Lib/selectors.py
Outdated
| """Poll-based selector.""" | ||
| def __init__(self): | ||
| super().__init__() | ||
| self._poller = self._poller() |
Done
Lib/selectors.py
Outdated
| poller_events |= self._EVENT_WRITE | ||
| try: | ||
| self._poller.register(key.fd, poller_events) | ||
| except BaseException: |
Done.
Lib/selectors.py
Outdated
| ready.append((key, events & key.events)) | ||
| return ready | ||
| def close(self): | ||
| if hasattr(self._poller, "close"): |
I overrode close() in EpollSelector and DevPollSelector to close the underlying FD as per your suggestion.
Lib/selectors.py
Outdated
| self._devpoll = select.devpoll() | ||
| _poller = select.devpoll | ||
| _EVENT_READ = select.POLLIN | ||
| _EVENT_WRITE = select.POLLOUT | ||
|
|
||
| def fileno(self): | ||
| return self._devpoll.fileno() |
Good catch. Fixed.
Great!
|
Cool. Merged. I'll submit a PR for a faster |
|
Congrats giampaolo for fixing that old issue :-) I hope that it will make
further selectors enhancements simpler.
|
|
Thanks Victor. ;) |
* #30014: refactor poll-related classes so that poll(), epoll() and devpoll() share the same methods for register(), unregister(), close() and select() * remove unused attribute * use specific class attributes instead of select.* constants * have all classes except SelectSelector a _selector attribute * BaseException -> Exception * be explicit in defining a close() method only for selectors which have it * fix AttributeError
Refactor poll-related classes so that poll(), epoll() and devpoll() share the same methods for register(), unregister(), close() and select().
This was inspired by http://bugs.python.org/issue30014 and serves as a strategy to avoid duplicating the implementation of modify() method.
UPDATE 2017-05-20: also use a single
self._pollerattribute for all classes instead of havingself._poll,self._kqueueetc..@Haypo
The text was updated successfully, but these errors were encountered: