#30014: refactor poll-related classes #1035

giampaolo · 2017-04-07T19:23:19Z

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._poller attribute for all classes instead of having self._poll, self._kqueue etc..

@Haypo

…poll() share the same methods for register(), unregister(), close() and select()

cf-natali · 2017-04-29T09:35:41Z

Lib/selectors.py

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

cf-natali · 2017-04-29T09:36:41Z

Lib/selectors.py

+            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

cf-natali · 2017-04-29T09:37:02Z

Lib/selectors.py

+        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:

cpython/Lib/selectors.py

Line 423 in 2e576f5

except OSError:

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

cf-natali · 2017-04-29T09:39:31Z

Lib/selectors.py

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

cf-natali · 2017-04-29T09:42:27Z

Lib/selectors.py

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

I overrode close() in EpollSelector and DevPollSelector to close the underlying FD as per your suggestion.

cf-natali

Sorry for the delay, didn't see this message.

With the proposed changes, sounds great!

…e it

giampaolo

Sorry for the delay, didn't see this message.

No problem. ;-)
OK, I think it's ready.

giampaolo · 2017-05-20T08:18:40Z

Lib/selectors.py

-        """Poll-based selector."""
+    def __init__(self):
+        super().__init__()
+        self._poller = self._poller()


giampaolo · 2017-05-20T08:19:01Z

Lib/selectors.py

+            poller_events |= self._EVENT_WRITE
+        try:
+            self._poller.register(key.fd, poller_events)
+        except BaseException:


giampaolo · 2017-05-20T08:30:17Z

Lib/selectors.py

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

giampaolo · 2017-05-20T08:31:43Z

Lib/selectors.py

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

cf-natali

Great!

giampaolo · 2017-05-20T09:35:40Z

Cool. Merged. I'll submit a PR for a faster modify() soon.

vstinner · 2017-05-20T16:43:56Z

Congrats giampaolo for fixing that old issue :-) I hope that it will make further selectors enhancements simpler.

giampaolo · 2017-05-20T18:37:17Z

Thanks Victor. ;)

#30014: refactor poll-related classes so that poll(), epoll() and dev…

29b6fee

…poll() share the same methods for register(), unregister(), close() and select()

giampaolo requested a review from 1st1 April 7, 2017 19:23

the-knights-who-say-ni added the CLA signed label Apr 7, 2017

giampaolo added 2 commits April 7, 2017 21:26

remove unused attribute

ea8820f

use specific class attributes instead of select.* constants

0b46cfe

cf-natali reviewed Apr 29, 2017

View reviewed changes

Merge branch 'master' into selectors-refactoring

28e7b35

cf-natali reviewed May 20, 2017

View reviewed changes

giampaolo added 5 commits May 20, 2017 10:08

Merge branch 'master' into selectors-refactoring

3ccc17c

have all classes except SelectSelector a _selector attribute

a56080d

BaseException -> Exception

27803ad

be explicit in defining a close() method only for selectors which hav…

3b8e310

…e it

fix AttributeError

33f14c7

giampaolo commented May 20, 2017

View reviewed changes

cf-natali approved these changes May 20, 2017

View reviewed changes

giampaolo merged commit 62c7d90 into master May 20, 2017

giampaolo deleted the selectors-refactoring branch May 20, 2017 09:34

gvanrossum mentioned this pull request Jun 12, 2017

bpo-30624 / selectors: use bare except clause #2082

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#30014: refactor poll-related classes #1035

#30014: refactor poll-related classes #1035

giampaolo commented Apr 7, 2017 •

edited

cf-natali Apr 29, 2017 •

edited

giampaolo May 1, 2017

giampaolo May 1, 2017

cf-natali May 20, 2017

giampaolo May 20, 2017

cf-natali Apr 29, 2017

giampaolo May 1, 2017

giampaolo May 20, 2017

cf-natali Apr 29, 2017

giampaolo May 1, 2017

cf-natali May 20, 2017 •

edited

cf-natali Apr 29, 2017

giampaolo May 20, 2017

cf-natali Apr 29, 2017

giampaolo May 1, 2017

cf-natali May 20, 2017

giampaolo May 20, 2017

cf-natali left a comment

giampaolo left a comment

giampaolo May 20, 2017

giampaolo May 20, 2017

giampaolo May 20, 2017

giampaolo May 20, 2017

cf-natali left a comment

giampaolo commented May 20, 2017

vstinner commented May 20, 2017 via email

giampaolo commented May 20, 2017

#30014: refactor poll-related classes #1035

#30014: refactor poll-related classes #1035

Conversation

giampaolo commented Apr 7, 2017 • edited

cf-natali Apr 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cf-natali May 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cf-natali left a comment

Choose a reason for hiding this comment

giampaolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cf-natali left a comment

Choose a reason for hiding this comment

giampaolo commented May 20, 2017

vstinner commented May 20, 2017 via email

giampaolo commented May 20, 2017

giampaolo commented Apr 7, 2017 •

edited

cf-natali Apr 29, 2017 •

edited

cf-natali May 20, 2017 •

edited