#30014: refactor poll-related classes by giampaolo · Pull Request #1035 · python/cpython

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

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

+            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

+        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

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

-                    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

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


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

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


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

-                    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

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

Uh oh!

Uh oh!

Conversation

giampaolo commented Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cf-natali Apr 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cf-natali May 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cf-natali left a comment

Choose a reason for hiding this comment

Uh oh!

giampaolo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cf-natali left a comment

Choose a reason for hiding this comment

Uh oh!

giampaolo commented May 20, 2017

Uh oh!

vstinner commented May 20, 2017 via email

Uh oh!

giampaolo commented May 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

giampaolo commented Apr 7, 2017 •

edited

Loading

cf-natali Apr 29, 2017 •

edited

Loading

cf-natali May 20, 2017 •

edited

Loading