python / cpython

The Python programming language
https://www.python.org
Other
62.99k stars 30.16k forks source link

selector.EpollSelector: EPOLLEXCLUSIVE, round 2 #89114

Open f0574f4f-5d0a-4e44-a148-ae46949abb82 opened 3 years ago

f0574f4f-5d0a-4e44-a148-ae46949abb82 commented 3 years ago
BPO 44951
Nosy @dgilman
PRs
  • python/cpython#27819
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['library'] title = 'selector.EpollSelector: EPOLLEXCLUSIVE, round 2' updated_at = user = 'https://github.com/dgilman' ``` bugs.python.org fields: ```python activity = actor = 'David.Gilman' assignee = 'none' closed = False closed_date = None closer = None components = ['Library (Lib)'] creation = creator = 'David.Gilman' dependencies = [] files = [] hgrepos = [] issue_num = 44951 keywords = ['patch'] message_count = 3.0 messages = ['399880', '399881', '399912'] nosy_count = 1.0 nosy_names = ['David.Gilman'] pr_nums = ['27819'] priority = 'normal' resolution = None stage = 'patch review' status = 'open' superseder = None type = None url = 'https://bugs.python.org/issue44951' versions = [] ```

    f0574f4f-5d0a-4e44-a148-ae46949abb82 commented 3 years ago

    Note that this is a different approach from the one taken in https://bugs.python.org/issue35517 although the issue is still the same.

    I've written a patch that allows users of selector.EpollSelector to enable EPOLLEXCLUSIVE on their file descriptors. This PR adds a setter and read only property to only the EpollSelector class instead of trying to expand the entire selector API like the other patch. The other discussion mentioned that there are some useful flags that could be passed down like this one. If other useful behavioral flags emerged in the future I think they should get their own API similar to how I've done it here. However, the other flags available so far for epoll are not useful for the selector module: EPOLLONESHOT and EPOLLET are incompatible with the design of the selector API and EPOLLWAKEUP is only marginally useful, not even getting exported into the select module after nearly a decade (Linux 3.5 was released in 2012).

    My API uses a getter/method instead of a read/write property because my understanding is that property access shouldn't raise exceptions, but if that doesn't matter here, it could be a read/write property.

    Justification:

    First, this is a useful flag that improves performance of epoll under even moderate load. I was going to turn it on by default in this patch but unfortunately Linux prevents you from doing epoll_mod() on anything that has EPOLLEXCLUSIVE set on it, breaking the python-level API. With this patch if you try to modify() after EPOLLEXCLUSIVE is set you'll get an EINVAL but I think the trade-off here is worth it. You don't enable EPOLLEXCLUSIVE on accident and you're reading the manpage for EPOLLEXCLUSIVE where this exact behavior is mentioned before turning anything on, right? And of course the Python docs also warn you about modify().

    Second, the thundering herd problem solved by EPOLLEXCLUSIVE is somewhat of a sore spot for Python's PR. In the past year two widely disseminated articles have brought up this issue. This PR isn't going to be a silver bullet however it can make a huge impact in gunicorn, the 3rd party library mentioned in both articles. Gunicorn is a popular WSGI web server and its gthread worker (not the default but the one most often used in production) directly uses the selector module from the standard library. Honestly, it's pretty cool that they were able to make such efficient use of a standard library module like this - how far we've come from the days of asynchat! There is nothing in gunicorn's threaded worker that calls modify() so there would be no API breakage there.

    Gunicorn thundering herd articles: https://blog.clubhouse.com/reining-in-the-thundering-herd-with-django-and-gunicorn/ https://rachelbythebay.com/w/2020/03/07/costly/

    f0574f4f-5d0a-4e44-a148-ae46949abb82 commented 3 years ago

    I also played with making another whole subclass that has it on by default, see this package https://github.com/dgilman/selector-epoll-exclusive

    That class could have EPOLLEXCLUSIVE on by default but could raise NotImplemented if you try and modify() it. Putting it in as a second class means you can also drop it into existing code unmodified, something that will play very nicely with all existing users of selector.

    f0574f4f-5d0a-4e44-a148-ae46949abb82 commented 3 years ago

    Reflecting on this a bit I wonder if the best of all worlds is to have an EpollExclusiveSelector whose modify() implementation just unregisters and registers the file descriptor.