Issue 1657: epoll and kqueue wrappers for the select module (original) (raw)
Created on 2007-12-19 06:57 by christian.heimes, last changed 2022-04-11 14:56 by admin. This issue is now closed.
Messages (38)
Author: Christian Heimes (christian.heimes) * 
Date: 2007-12-19 09:41
UPDATE:
- Better API with register(), unregister() and modify() instead of control()
- Some documentation
Author: Thomas Herve (therve) *
Date: 2007-12-19 09:47
Cool, thanks for working on that. Just for the record, I don't really understand the workflow: why closing the other ticket as duplicate and not post the patch on the old one? But whatever.
For this patch, I don't see the benefit of putting it in the select module, instead of a separate module. Is there a specific reason?
Looking at the code, I don't have many remarks. pyepoll_new may leak if epoll_create fails. I think that allowing threading around epoll_ctl is useless, but I may be wrong.
Thanks again for working on it.
Author: Christian Heimes (christian.heimes) *
Date: 2007-12-19 10:50
For this patch, I don't see the benefit of putting it in the select module, instead of a separate module. Is there a specific reason?
There is at least one, but probably several other modules named "epoll" or "_epoll" in the wild. These modules implement an epoll interface with Pyrex, ctypes or C. All of them have a slightly different API. I don't want to break 3rd party software.
The select module already contains an interface to select and poll. IMO it's the best place.
Looking at the code, I don't have many remarks. pyepoll_new may leak if epoll_create fails. I think that allowing threading around epoll_ctl is useless, but I may be wrong.
Thanks, I've fixed the problem in pyepoll_new.
The Linux kernel protects every call to epoll_ctl with a mutex. It could block and therefor I'm allowing threads around the epoll_ctl() call.
Author: Christian Heimes (christian.heimes) *
Date: 2007-12-19 17:44
I mistakenly removed the wrong message. Here is the original msg:
The patch implements Linux's epoll interface (http://linux.die.net/man/4/epoll). My patch doesn't introduce a new module like http://bugs.python.org/issue1675118 and it wraps the epoll control fd in an object like Twisted's _epoll.pyd interface.
My interface is almost identical to Twisted's API except for some names. I named my control function "control" instead of "_control" and the constants are all named "select.EPOLL_SPAM" instead of "SPAM".
Missing: Documentation
Author: Christian Heimes (christian.heimes) *
Date: 2007-12-19 17:45
Added license header to test_epoll. Some C code cleanups
Author: Christian Heimes (christian.heimes) *
Date: 2007-12-20 10:25
First draft of a kqueue wrapper loosely based on Twisted's _kqueue.c from the kqreactor branch
Author: Christian Heimes (christian.heimes) *
Date: 2007-12-20 14:44
UPDATE:
- misc cleanups
- added missing constants, see man kqueue(2)
Author: Thomas Herve (therve) *
Date: 2007-12-20 18:34
Some remarks:
- the name of the function used for PyArg_ParseTupleAndKeywords in register, modify, unregister is set to control instead of the good name.
- there is a leak in pyepoll_new if the parsing of arguments fails.
- the indentation is sometimes tabs, sometimes spaces. That should be good to unify this (to tabs I guess, since the select module used tabs before).
- it seems there is an unrelated change in sunau.py
- I don't think the stdlib unittest module has skip support. You have to find another way to skip the tests if the modules aren't present.
I've been able to port the epollreactor to your implementation and run the whole twisted tests with it, so I don't think there are outstanding problems. The code is fairly simple anyway.
That's it for epoll and general remarks. I'll look at kqueue asap. Thanks!
Author: Christian Heimes (christian.heimes) *
Date: 2007-12-20 19:58
Some remarks:
- the name of the function used for PyArg_ParseTupleAndKeywords in register, modify, unregister is set to control instead of the good name.
Fixed
- there is a leak in pyepoll_new if the parsing of arguments fails.
Fixed
- the indentation is sometimes tabs, sometimes spaces. That should be good to unify this (to tabs I guess, since the select module used tabs before).
Fixed except for switch() and goto. I find the 4 space indention of the case and the goto lables easier to read.
- it seems there is an unrelated change in sunau.py
Fixed
- I don't think the stdlib unittest module has skip support. You have to find another way to skip the tests if the modules aren't present.
Fixed ;)
I've been able to port the epollreactor to your implementation and run the whole twisted tests with it, so I don't think there are outstanding problems. The code is fairly simple anyway.
That's it for epoll and general remarks. I'll look at kqueue asap. Thanks!
Thansk for the code review and your remarks. I've fixed the problems locally. I've also fixed a problem in unregister when fd is already closed and I'm using PyFile_AsFileDescriptor(). It supports ints and objects with a fileno() method.
I'm waiting for your test of kqueue before I upload the patch.
Author: Christian Heimes (christian.heimes) *
Date: 2007-12-20 22:40
Here is the promised patch. I've added a small optimization. The objects now keep a buffer to reduce the malloc overhead.
Author: Christian Heimes (christian.heimes) *
Date: 2007-12-21 09:41
I've fixed a bug with the preallocated buffer and in the repr() of kevent.
Author: Thomas Herve (therve) *
Date: 2007-12-21 10:36
Here I go for kqueue:
- the docstring of test_kqueue.py is wrong
- the tests are a bit light. It would be good the have a test like test_control_and_wait in test_epoll.
- the kqueue_queue_control (and the pyepoll_poll) are now completely wrong! You should not limit to FD_SETSIZE, these 2 systems are there because they're able to handle for fds than that. Also, this buffer thing looks like a premature optimization. I'm unable to tell if it's correct or not.
- the NETDEV and related flags aren't defined under OS X 10.4. I guess there are flags for freebsd, but kqueue should build on OS X too.
I've been able to use this module for the twisted reactor, so the functionality is OK. But thsi FD_SETSIZE limit is a huge problem.
Author: Christian Heimes (christian.heimes) *
Date: 2007-12-21 13:07
- the docstring of test_kqueue.py is wrong
Fixed
- the tests are a bit light. It would be good the have a test like test_control_and_wait in test_epoll.
Does Twisted have additional tests? I agree that the tests are too light weighted but I don't have spare time to write more tests. I may have more time in a few days after xmas.
- the kqueue_queue_control (and the pyepoll_poll) are now completely wrong! You should not limit to FD_SETSIZE, these 2 systems are there because they're able to handle for fds than that. Also, this buffer thing looks like a premature optimization. I'm unable to tell if it's correct or not.
I've seen the limitation in an example somewhere. I've read the specs several times and I think you are right. I now use FD_SETSIZE as sizehint for epoll() but I don't limit the amount of fds any more.
- the NETDEV and related flags aren't defined under OS X 10.4. I guess there are flags for freebsd, but kqueue should build on OS X too.
I'm using FreeBSD to test the kqueue interface. I can't tell if it's working on Mac OS X. I've put the NETDEV related macros in an ifdef block.
The new patch replaces the FD_SETSIZE limitation and adds a richcompare slot to kevent. Do we need an alternative constructor which creates an epoll and kqueue object from a given fd?
Author: Thomas Herve (therve) *
Date: 2007-12-21 16:28
I attached a patch with a more complete test of kqueue. It's not that great, but it's a thing. I've only tested on OS X, but it works.
Regarding the ability of building an epoll object from a fd, it might be usefull in some corner cases, but that's not a priority.
exarkun looked at the patch and told me that there may be some threadsafety issues: for example, when calling epoll_wait, you use self->evs unprotected. It's not very important, but you may want to tell it in the documentation.
As you started the rich comparison for kevent objects, it may be interesting to have full comparison (to sort list of events). It's not a high priority though.
That's all for now!
Author: Christian Heimes (christian.heimes) *
Date: 2007-12-22 11:09
I attached a patch with a more complete test of kqueue. It's not that great, but it's a thing. I've only tested on OS X, but it works.
A small unit test is better than no unit test :)
Regarding the ability of building an epoll object from a fd, it might be usefull in some corner cases, but that's not a priority.
It should be trivial to add an epoll.fromfd() classmethod.
exarkun looked at the patch and told me that there may be some threadsafety issues: for example, when calling epoll_wait, you use self->evs unprotected. It's not very important, but you may want to tell it in the documentation.
I found an interesting article about epoll. It states that epoll_wait() is thread safe. Ready lists of two parallel threds never contain the same fd. http://lwn.net/Articles/224240/ It's easier to remove the buffer and allocate memory inside the wait() method than to add semaphores. It makes the code.
As you started the rich comparison for kevent objects, it may be interesting to have full comparison (to sort list of events). It's not a high priority though.
What do you suggest as sort criteria?
Author: Thomas Herve (therve) *
Date: 2007-12-22 16:18
What do you suggest as sort criteria?
The natural sort of the tuple you used for equality, I'd say.
Author: Christian Heimes (christian.heimes) *
Date: 2007-12-23 21:07
I had some trouble with your patch. BSD doesn't raise a socket error with EINPROGRESS and it sets the ENABLE and ADD flags to 0.
The new patch removes the preallocated buffer and adds fromfd() class methods. kevent objects are fully orderable, too.
Author: Thomas Herve (therve) *
Date: 2007-12-24 10:50
You have to use sys.platform to get 'darwin', not os.name. The rest of the test seems good.
I didn't spot the check of EEXIST in pyepoll_internal_ctl, I'm not sure it's a good idea. I understand it's for being able to only use register, but it's not the way it's meant to be used. At least, there should be a test for it.
Your example in kqueue_queue_doc doesn't work:
- it uses KQ_ADD instead of KQ_EV_ADD
- on OS X, you can't use kqueue on stdin
- it uses KQ_DELETE instead of KQ_EV_DELETE Maybe an example on an arbitrary fd would be better.
FWIW, I would have prefer to review epoll wrapper first, then kqueue. Splitting functionalities makes it easier to review. But that will be great to have that in python :).
Author: Christian Heimes (christian.heimes) *
Date: 2007-12-25 17:13
UPDATE:
- Removed EEXIST magic
- timeout is now milliseconds but supports float for smaller timeouts
- poll object has a modify method, too
Author: Christian Heimes (christian.heimes) *
Date: 2008-01-07 20:39
Guido, have you reviewed the patch and are you fine with it?
Author: Guido van Rossum (gvanrossum) *
Date: 2008-01-07 20:43
Not yet, I ran out of time. Can you hold on for another week?
Author: Christian Heimes (christian.heimes) *
Date: 2008-01-19 14:45
Can somebody else review the patch? therve from the Twisted team has reviewed it but I like to get an opinion of another core developer. Guido seems to be too busy.
Author: Guido van Rossum (gvanrossum) *
Date: 2008-01-19 19:53
Still haven't had the time (sorry!), but one comment: please don't specify timeouts in millisecond. We use seconds (floats if necessary) everywhere else in Python, regardless of the underlying data structure or resolution.
Author: Christian Heimes (christian.heimes) *
Date: 2008-01-20 09:01
Yeah, it's a reasonable suggestion. I'm changing the code to seconds as positive float and None = blocking.
Author: Christian Heimes (christian.heimes) *
Date: 2008-02-24 13:48
I've updated the patch. The latest patch didn't contain the unit tests and it failed to apply cleanly, too.
Author: Thomas Herve (therve) *
Date: 2008-02-29 08:45
Is there a chance for this go in the first alpha? FWIW, I've tested it with twisted kqueue and epoll reactors, and didn't get any problems.
There are still 2 typos in the patch: KQ_ADD is used 2 times in the docs instead of KQ_EV_ADD. Everything else looks good to me.
Author: Christian Heimes (christian.heimes) *
Date: 2008-02-29 09:08
I love to get it into the next alpha but I don't have time to today. Can you take it to the mailing list and ask somebody to review and submit the patch?
Author: Trent Nelson (trent) *
Date: 2008-03-20 02:15
Patch applies cleanly on FreeBSD 6.2-STABLE and all tests pass. Can't comment on technical accuracy.
Author: Gregory P. Smith (gregory.p.smith) *
Date: 2008-03-20 06:44
+1
trunk_select_epoll_kqueue9.patch looks good to me.
style nit: I'd just use self.fail("error message") instead of raise AssertionError("error message") within unittests. regardless, both work so I don't care. :)
Author: Jim Jewett (jimjjewett)
Date: 2008-03-20 20:50
Is pyepoll a good prefix? To me, it looks a lot like the _Py and Py reservered namespaces, but not quite...
Author: Christian Heimes (christian.heimes) *
Date: 2008-03-20 20:58
I had to use some kind of prefix to avoid naming collisions with the epoll_* functions for the epoll header file. pyepoll sounded reasonable to me.
Author: Guido van Rossum (gvanrossum) *
Date: 2008-03-21 22:05
pyepoll for static names sounds fine (assuming you want some consistency).
Given all the rave reviews, what are the chances that you'll be checking this in soon?
Author: Christian Heimes (christian.heimes) *
Date: 2008-03-21 23:00
Say "Go" and I'll check the patch in ASAP.
Author: Guido van Rossum (gvanrossum) *
Date: 2008-03-21 23:21
Go.
Author: Christian Heimes (christian.heimes) *
Date: 2008-03-21 23:51
I've applied the patch in r61722.
TODO: finish the documentation, any help is appreciated
Author: Erik Gorset (Erik Gorset)
Date: 2009-06-21 21:59
The kqueue implementation is not working. It has a silly bug:
chl[i] = ((kqueue_event_Object *)ei)->e;
chl[i++] = ((kqueue_event_Object *)ei)->e;
I've created issue 5910 and included a patch, which also adds another test case. Anything else I need to do to get the patch accepted?
Author: A.M. Kuchling (akuchling) *
Date: 2010-02-22 16:00
What exactly needs to be finished in the documentation? There are sections for the epoll and kqueue objects, and the epoll section looks fine, if brief. Is the problem that the kqueue section says things like 'filter-specific data' with no explanation?
Author: Terry J. Reedy (terry.reedy) *
Date: 2010-07-10 23:43
"Is the problem that the kqueue section says things like 'filter-specific data' with no explanation?"
The phase is not in the (newer) 3.1.2 docs. Given the absence of a specific doc issue, closing.
History
Date
User
Action
Args
2022-04-11 14:56:29
admin
set
github: 45998
2010-07-10 23:43:25
terry.reedy
set
status: open -> closed
title: [patch] epoll and kqueue wrappers for the select module -> epoll and kqueue wrappers for the select module
keywords: - patch
nosy: + terry.reedy
versions: + Python 3.1, Python 2.7, Python 3.2, - Python 3.0
messages: +
resolution: accepted -> fixed
stage: resolved
2010-02-22 16:00:55
akuchling
set
nosy: + akuchling
messages: +
2009-06-21 21:59:20
Erik Gorset
set
nosy: + Erik Gorset
messages: +
2009-03-25 06:06:33
intgr
set
nosy: + intgr
2008-03-21 23:51:09
christian.heimes
set
resolution: accepted
messages: +
components: + Documentation, - Extension Modules
2008-03-21 23:21:50
gvanrossum
set
messages: +
2008-03-21 23:00:26
christian.heimes
set
messages: +
2008-03-21 22:05:32
gvanrossum
set
messages: +
2008-03-20 20:58:50
christian.heimes
set
messages: +
2008-03-20 20:50:17
jimjjewett
set
nosy: + jimjjewett
messages: +
2008-03-20 06:44:58
gregory.p.smith
set
nosy: + gregory.p.smith
messages: +
2008-03-20 02:15:15
trent
set
nosy: + trent
messages: +
2008-02-29 09:08:45
christian.heimes
set
messages: +
2008-02-29 08:45:11
therve
set
messages: +
2008-02-24 13:49:10
christian.heimes
set
files: - trunk_select_epoll_kqueue8.patch
2008-02-24 13:49:04
christian.heimes
set
files: - trunk_select_epoll_kqueue6.patch
2008-02-24 13:49:00
christian.heimes
set
files: - trunk_select_epoll_kqueue7.patch
2008-02-24 13:48:55
christian.heimes
set
files: - trunk_select_epoll_kqueue5.patch
2008-02-24 13:48:42
christian.heimes
set
files: + trunk_select_epoll_kqueue9.patch
messages: +
2008-02-04 15:11:47
giampaolo.rodola
set
nosy: + giampaolo.rodola
2008-01-20 10:16:46
christian.heimes
set
files: + trunk_select_epoll_kqueue8.patch
2008-01-20 09:01:49
christian.heimes
set
messages: +
2008-01-19 19:53:18
gvanrossum
set
messages: +
2008-01-19 14:45:25
christian.heimes
set
messages: +
2008-01-07 20:43:00
gvanrossum
set
messages: +
2008-01-07 20:39:15
christian.heimes
set
nosy: + gvanrossum
messages: +
components: + Extension Modules
2008-01-06 22:29:44
admin
set
keywords: - py3k
versions: Python 2.6, Python 3.0
2007-12-25 17:13:17
christian.heimes
set
files: + trunk_select_epoll_kqueue7.patch
messages: +
2007-12-24 10:50:04
therve
set
messages: +
2007-12-23 21:07:29
christian.heimes
set
files: + trunk_select_epoll_kqueue6.patch
messages: +
2007-12-22 16🔞36
therve
set
messages: +
2007-12-22 11:09:05
christian.heimes
set
messages: +
2007-12-21 16:28:59
therve
set
messages: +
2007-12-21 16:19:07
therve
set
files: + test_kqueue.diff
2007-12-21 13:07:37
christian.heimes
set
files: - trunk_select_epoll_kqueue4.patch
2007-12-21 13:07:29
christian.heimes
set
files: + trunk_select_epoll_kqueue5.patch
messages: +
2007-12-21 10:36:27
therve
set
messages: +
2007-12-21 09:41:48
christian.heimes
set
files: + trunk_select_epoll_kqueue4.patch
messages: +
2007-12-21 09:41:02
christian.heimes
set
files: - trunk_select_epoll_kqueue3.patch
2007-12-21 09:40:58
christian.heimes
set
files: - trunk_select_epoll_kqueue2.patch
2007-12-21 09:40:53
christian.heimes
set
files: - trunk_select_epoll_kqueue.patch
2007-12-20 22:40:48
christian.heimes
set
files: + trunk_select_epoll_kqueue3.patch
messages: +
2007-12-20 19:58:38
christian.heimes
set
messages: +
2007-12-20 18:34:20
therve
set
messages: +
2007-12-20 14:44:00
christian.heimes
set
files: + trunk_select_epoll_kqueue2.patch
messages: +
2007-12-20 12:44:55
christian.heimes
set
files: + trunk_select_epoll_kqueue.patch
2007-12-20 12:44:41
christian.heimes
set
files: - trunk_select_epoll_kqueue.patch
2007-12-20 12:44:37
christian.heimes
set
files: - trunk_select_epoll3.patch
2007-12-20 10:25:59
christian.heimes
set
files: + trunk_select_epoll_kqueue.patch
messages: +
title: [patch] select.epoll wrapper for Linux 2.6 epoll() -> [patch] epoll and kqueue wrappers for the select module
2007-12-19 17:45:02
christian.heimes
set
files: + trunk_select_epoll3.patch
messages: +
2007-12-19 17:44:15
christian.heimes
set
files: - trunk_select_epoll2.patch
2007-12-19 17:44:11
christian.heimes
set
messages: +
2007-12-19 17:43:16
christian.heimes
set
messages: -
2007-12-19 17:43:12
christian.heimes
set
files: - trunk_select_epoll.patch
2007-12-19 10:50:49
christian.heimes
set
messages: +
2007-12-19 09:47:30
therve
set
nosy: + therve
messages: +
2007-12-19 09:41:03
christian.heimes
set
files: + trunk_select_epoll2.patch
messages: +
2007-12-19 06:58:35
christian.heimes
link
2007-12-19 06:57:08
christian.heimes
create