Issue35517
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2018-12-17 10:31 by Manjusaka, last changed 2022-04-11 14:59 by admin.
Pull Requests | |||
---|---|---|---|
URL | Status | Linked | Edit |
PR 11193 | open | Manjusaka, 2018-12-17 11:27 |
Messages (20) | |||
---|---|---|---|
msg331969 - (view) | Author: Manjusaka (Manjusaka) * | Date: 2018-12-17 10:31 | |
Add a keyword argument for selector.EpollSelector with default value. This can help people use the EPOLLEXCLUSIVE since Python 3.7 and Linux Kernel 4.5 to avoid the herd effect like this def register(self, fileobj, events, data=None, exclusive=False): key = super().register(fileobj, events, data) epoll_events = 0 if events & EVENT_READ: epoll_events |= select.EPOLLIN if events & EVENT_WRITE: epoll_events |= select.EPOLLOUT try: if exclusive and hasattr(select, "EPOLLEXCLUSIVE"): epoll_events |= select.EPOLLEXCLUSIVE self._epoll.register(key.fd, epoll_events) except BaseException: super().unregister(fileobj) raise return key |
|||
msg331970 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2018-12-17 10:34 | |
> Add a keyword argument for selector.EpollSelector with default value. I suggest to use a keyword-only parameter: def register(self, fileobj, events, data=None, *, exclusive=False): Do you want to work on a pull request? |
|||
msg331971 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2018-12-17 10:36 | |
EPOLLEXCLUSIVE doc from Linux manual page: """ EPOLLEXCLUSIVE (since Linux 4.5) Sets an exclusive wakeup mode for the epoll file descriptor that is being attached to the target file descriptor, fd. When a wakeup event occurs and multiple epoll file descriptors are attached to the same target file using EPOLLEXCLUSIVE, one or more of the epoll file descriptors will receive an event with epoll_wait(2). The default in this scenario (when EPOLLEXCLU‐ SIVE is not set) is for all epoll file descriptors to receive an event. EPOLLEXCLUSIVE is thus useful for avoiding thundering herd problems in certain scenarios. If the same file descriptor is in multiple epoll instances, some with the EPOLLEXCLUSIVE flag, and others without, then events will be provided to all epoll instances that did not specify EPOLLEXCLUSIVE, and at least one of the epoll instances that did specify EPOLLEXCLUSIVE. The following values may be specified in conjunction with EPOLLEXCLUSIVE: EPOLLIN, EPOLLOUT, EPOLLWAKEUP, and EPOLLET. EPOLLHUP and EPOLLERR can also be specified, but this is not required: as usual, these events are always reported if they occur, regardless of whether they are specified in events. Attempts to specify other values in events yield an error. EPOLLEXCLUSIVE may be used only in an EPOLL_CTL_ADD operation; attempts to employ it with EPOLL_CTL_MOD yield an error. If EPOLLEXCLUSIVE has been set using epoll_ctl(), then a subsequent EPOLL_CTL_MOD on the same epfd, fd pair yields an error. A call to epoll_ctl() that specifies EPOLLEXCLUSIVE in events and spec‐ ifies the target file descriptor fd as an epoll instance will likewise fail. The error in all of these cases is EINVAL. """ See also https://lwn.net/Articles/633422/ |
|||
msg331972 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2018-12-17 10:36 | |
if exclusive and hasattr(select, "EPOLLEXCLUSIVE"): epoll_events |= select.EPOLLEXCLUSIVE Maybe NotImplementedError would be more appropriate rather than silently ignore the error? |
|||
msg331973 - (view) | Author: Manjusaka (Manjusaka) * | Date: 2018-12-17 10:37 | |
I will work on a PR |
|||
msg331974 - (view) | Author: Manjusaka (Manjusaka) * | Date: 2018-12-17 10:38 | |
"a keyword-only parameter" is good I'll take it done |
|||
msg331977 - (view) | Author: Giampaolo Rodola' (giampaolo.rodola) * ![]() |
Date: 2018-12-17 10:59 | |
I'm not sure I understand what EPOLLEXCLUSIVE is about. Could you provide a use case? Also, there are other constants which may also be useful such as EPOLLWAKEUP and EPOLLONESHOT: http://man7.org/linux/man-pages/man2/epoll_ctl.2.html So perhaps it makes more sense to expose a lower-level "extra_events" argument instead, which is more flexible and also saves us from the hassle of describing what the new argument is about (which really is a Linux kernel thing) in the doc: >>> s.register(fd, EVENT_READ, extra_events=select.EPOLLEXCLUSIVE | select.EPOLLONESHOT) That raises the question whether we should also have an "extra_events" argument for modify() method (probably yes). But again, I think it makes sense to understand what's the use case of these constants first, because if they are rarely used maybe who needs them can simply look at the source and use s._selector.modify() directly. |
|||
msg331979 - (view) | Author: Manjusaka (Manjusaka) * | Date: 2018-12-17 11:09 | |
Hello rodola Here's detail: In the server, we use multiprocess to handle the request with share the same FD. OK, when a request comes, all the process wake up and try to accept the request but only one process will accept success and the others will raise an Exception if we use the epoll without EPOLLEXCLUSIVE. That means there will waste the performance. This effect is named thundering herd. But if we use epoll with EPOLLEXCLUSIVE, when the envents happen, only one process will wake and try to handle the event. So, I think it's necessary to support the EPOLLEXCLUSIVE in selectors.EpollSelector Actually, Python support EPOLLEXCLUSIVE official since 3.7. It's time to support it in selectors |
|||
msg331991 - (view) | Author: Giampaolo Rodola' (giampaolo.rodola) * ![]() |
Date: 2018-12-17 11:34 | |
I see. Then I would say it's a matter of deciding what's the best API to provide. Another possibility is to promote the underlying epoll() instance as a public property, so one can do: >>> s = selectors.EpollSelector() >>> s.register(fd, EVENT_READ) >>> s.selector.modify(fd, select.EPOLLEXCLUSIVE) That raises the question whether all selector classes should have a public "selector" attribute. poll() and devpoll() related classes may need it for POLLPRI, POLLRDHUP, POLLWRBAND or others (whatever their use case is). kqueue() also has it's own specific constants (KQ_FILTER_* and KQ_EV_*). The only one where a public "selector" property would be useless is SelectSelector. |
|||
msg331994 - (view) | Author: Manjusaka (Manjusaka) * | Date: 2018-12-17 11:42 | |
In my opinion selectors is an abstract for select, so I don't think allow people use select.* in selector is a good idea. like this > s.register(fd, EVENT_READ, extra_events=select.EPOLLEXCLUSIVE | select.EPOLLONESHOT) Because the multiple epoll's params are supported in different Python version and Linux Kernel version. So, I think it's a good idea to support different epoll's params by keyword-only param in register method. It's also convenient to check the params |
|||
msg331996 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2018-12-17 11:47 | |
I prefer Giampaolo since discussed flags are very specific to epoll(): select() doesn't support them for example, nor kqueue nor devpoll (not *yet*). If we add a keyword-parameter, to me, it sounds like it's something "portable" working on multiple platforms and then you need hasattr(): if exclusive and hasattr(select, "EPOLLEXCLUSIVE"): epoll_events |= select.EPOLLEXCLUSIVE If the caller pass select.EPOLLEXCLUSIVE, hasattr() is useless. Moreover, we directly support any EPOLL constant exposed in the select module. No need to change the API. |
|||
msg331998 - (view) | Author: Manjusaka (Manjusaka) * | Date: 2018-12-17 12:05 | |
OK, I will change my code |
|||
msg331999 - (view) | Author: Manjusaka (Manjusaka) * | Date: 2018-12-17 12:54 | |
Vicor: > Moreover, we directly support any EPOLL constant exposed in the select module. No need to change the API. I don't think so In class _PollLikeSelector ,here's register method def register(self, fileobj, events, data=None): key = super().register(fileobj, events, data) poller_events = 0 if events & EVENT_READ: poller_events |= self._EVENT_READ if events & EVENT_WRITE: poller_events |= self._EVENT_WRITE this code filter the event that people push in it. It only supports select.EPOLLIN and select.EPOLLOUT |
|||
msg332000 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2018-12-17 13:02 | |
I mean that using extra_events to get access to EPOLLEXCLUSIVE, there is no need later to add a new parameter for select.EPOLLONESHOT: it would be "future proof"! |
|||
msg332017 - (view) | Author: Giampaolo Rodola' (giampaolo.rodola) * ![]() |
Date: 2018-12-17 18:48 | |
I took a look at your PR. As the PR currently stands it only works with epoll() selector. For the other selectors this is just an extra argument which does nothing, so it complicates the API of 2 methods for no real gain. Also, a single argument is not compatible with kqueue() because kqueue() requires two distinct `KQ_FILTER_*` and `KQ_EV_*` constants which cannot be ORed together. Instead, I think simply turning the underlying selector instance into a public property is more flexible and less complex. On Linux you'll do: >>> s = selectors.EpollSelector() >>> s.register(fd, EVENT_READ) >>> s.selector.modify(fd, select.EPOLLEXCLUSIVE) ``` ...and on BSD: >>> s = selectors.KqueueSelector() >>> s.register(fd, EVENT_READ) >>> k = s.selector.kevent(fd, select.KQ_FILTER_READ, select.KQ_EV_CLEAR) >>> s.selector.control([k], 0, 0) ``` Alternatively we could just keep `DefaultSelector._selector` private and document it. Personally I'd also be OK with that even though I'm not sure if there if there are precedents of documenting private APIs. |
|||
msg332018 - (view) | Author: Manjusaka (Manjusaka) * | Date: 2018-12-17 19:06 | |
Actually, in my implementation, it also supports POLL with the different event. I don't think to make selector be a public property is a good idea. It will break the whole system integrity. Please think about it, if people want to use epoll/poll with some special event, they must use it like this. > s = selectors.EpollSelector() > s.selector.register(fd,select.EPOLLIN | select.EPOLLEXCLUSIVE) > s.selector.modify(fd,select.EPOLLOU | select.EPOLLEXCLUSIVE) Here's a question, why we support the register? I think it will make people don't care about the detail. So, as you say, it's a little bit complicated, but it will help people don't care about the selector property detail, they just use the argument when they want to use it. I think it's worth it. |
|||
msg332019 - (view) | Author: Andrew Svetlov (asvetlov) * ![]() |
Date: 2018-12-17 19:14 | |
Hmm, I forgot kqueue usage details: it operated not only with int flags but more complex structures. Giampaolo, thanks for pointing on! Please let me sleep on it. |
|||
msg332038 - (view) | Author: Xiang Zhang (xiang.zhang) * ![]() |
Date: 2018-12-18 02:50 | |
> I don't think to make selector be a public property is a good idea. It will break the whole system integrity. If exposing a private property is not a good idea, another choice may be construct a selector with a customized I/O multiplexer, adding an optional parameter to the __init__. But actually I'm -1 to this change. `selectors` makes underlying implementations irrelavant to most users since we can simply use `DefaultSelector`(maybe why only read/write events are valid now?). But you are seeking to add implementation specific details back. If you know you want to add EPOLL_EXCLUSIVE, why not just use `select.epoll`? A single selector doesn't do much more than the underlying multiplexer. |
|||
msg332039 - (view) | Author: Manjusaka (Manjusaka) * | Date: 2018-12-18 03:20 | |
> `selectors` makes underlying implementations irrelavant to most users since we can simply use `DefaultSelector` I agree with this. > If you know you want to add EPOLL_EXCLUSIVE, why not just use `select.epoll`? I don't think so. If I use the `select` directly, that means I have to give up all the features what `selectors` support such as binding a `data` to a fd, that also means I have to do some repeat work myself. I don't think it's a good idea. So, why not change this API when we can keep the compatibility? |
|||
msg334562 - (view) | Author: Manjusaka (Manjusaka) * | Date: 2019-01-30 14:43 | |
ping.... |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-11 14:59:09 | admin | set | github: 79698 |
2019-01-30 14:43:45 | Manjusaka | set | messages: + msg334562 |
2018-12-18 03:20:03 | Manjusaka | set | messages: + msg332039 |
2018-12-18 02:50:31 | xiang.zhang | set | nosy:
+ xiang.zhang messages: + msg332038 |
2018-12-17 19:16:55 | Manjusaka | set | nosy:
- asvetlov |
2018-12-17 19:14:50 | asvetlov | set | messages: + msg332019 |
2018-12-17 19:08:48 | gvanrossum | set | nosy:
- gvanrossum |
2018-12-17 19:06:17 | Manjusaka | set | messages: + msg332018 |
2018-12-17 18:48:45 | giampaolo.rodola | set | nosy:
+ gvanrossum, asvetlov messages: + msg332017 |
2018-12-17 13:02:48 | vstinner | set | messages: + msg332000 |
2018-12-17 12:54:07 | Manjusaka | set | messages: + msg331999 |
2018-12-17 12:05:07 | Manjusaka | set | messages: + msg331998 |
2018-12-17 11:47:11 | vstinner | set | messages: + msg331996 |
2018-12-17 11:42:06 | Manjusaka | set | messages: + msg331994 |
2018-12-17 11:34:29 | giampaolo.rodola | set | messages:
+ msg331991 stage: patch review -> |
2018-12-17 11:27:40 | Manjusaka | set | keywords:
+ patch stage: patch review pull_requests: + pull_request10433 |
2018-12-17 11:09:02 | Manjusaka | set | messages: + msg331979 |
2018-12-17 10:59:10 | giampaolo.rodola | set | nosy:
+ neologix, yselivanov messages: + msg331977 |
2018-12-17 10:38:19 | Manjusaka | set | messages: + msg331974 |
2018-12-17 10:37:46 | Manjusaka | set | messages: + msg331973 |
2018-12-17 10:36:49 | vstinner | set | messages: + msg331972 |
2018-12-17 10:36:01 | vstinner | set | messages: + msg331971 |
2018-12-17 10:34:08 | vstinner | set | nosy:
+ vstinner, giampaolo.rodola title: enhhance for selector.EpollSelector -> selector.EpollSelector: add new parameter to support EPOLLEXCLUSIVE messages: + msg331970 versions: - Python 3.7 |
2018-12-17 10:31:38 | Manjusaka | create |