classification
Title: epoll and kqueue wrappers for the select module
Type: enhancement Stage: resolved
Components: Documentation Versions: Python 3.2, Python 3.1, Python 2.7, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: christian.heimes Nosy List: Erik Gorset, akuchling, christian.heimes, giampaolo.rodola, gregory.p.smith, gvanrossum, intgr, jimjjewett, terry.reedy, therve, trent
Priority: normal Keywords:

Created on 2007-12-19 06:57 by christian.heimes, last changed 2010-07-10 23:43 by terry.reedy. This issue is now closed.

Files
File name Uploaded Description Edit
test_kqueue.diff therve, 2007-12-21 16:19
trunk_select_epoll_kqueue9.patch christian.heimes, 2008-02-24 13:48
Messages (38)
msg58800 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2007-12-19 09:41
UPDATE:

* Better API with register(), unregister() and modify() instead of control()
* Some documentation
msg58801 - (view) 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.
msg58804 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) 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.
msg58813 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) 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
msg58814 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2007-12-19 17:45
Added license header to test_epoll.
Some C code cleanups
msg58867 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2007-12-20 10:25
First draft of a kqueue wrapper loosely based on Twisted's _kqueue.c
from the kqreactor branch
msg58878 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2007-12-20 14:44
UPDATE:

* misc cleanups
* added missing constants, see man kqueue(2)
msg58914 - (view) 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!
msg58920 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) 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.
msg58929 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) 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.
msg58938 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2007-12-21 09:41
I've fixed a bug with the preallocated buffer and in the repr() of kevent.
msg58941 - (view) 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.
msg58944 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) 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?
msg58947 - (view) 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!
msg58961 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) 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?
msg58962 - (view) 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.
msg58976 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) 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.
msg58982 - (view) 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 :).
msg58994 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) 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
msg59486 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-01-07 20:39
Guido, have you reviewed the patch and are you fine with it?
msg59487 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2008-01-07 20:43
Not yet, I ran out of time. Can you hold on for another week?
msg60176 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) 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.
msg60222 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) 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.
msg60266 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) 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.
msg62899 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) 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.
msg63128 - (view) 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.
msg63129 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) 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?
msg64137 - (view) Author: Trent Nelson (trent) * (Python committer) Date: 2008-03-20 02:15
Patch applies cleanly on FreeBSD 6.2-STABLE and all tests pass.  Can't 
comment on technical accuracy.
msg64158 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) 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. :)
msg64207 - (view) 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...
msg64209 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) 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.
msg64287 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) 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?
msg64292 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-03-21 23:00
Say "Go" and I'll check the patch in ASAP.
msg64294 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2008-03-21 23:21
Go.
msg64299 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-03-21 23:51
I've applied the patch in r61722.

TODO: finish the documentation, any help is appreciated
msg89588 - (view) 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?
msg99763 - (view) Author: A.M. Kuchling (akuchling) * (Python committer) 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?
msg109945 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) 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
2010-07-10 23:43:25terry.reedysetstatus: 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: + msg109945
resolution: accepted -> fixed
stage: resolved
2010-02-22 16:00:55akuchlingsetnosy: + akuchling
messages: + msg99763
2009-06-21 21:59:20Erik Gorsetsetnosy: + Erik Gorset
messages: + msg89588
2009-03-25 06:06:33intgrsetnosy: + intgr
2008-03-21 23:51:09christian.heimessetresolution: accepted
messages: + msg64299
components: + Documentation, - Extension Modules
2008-03-21 23:21:50gvanrossumsetmessages: + msg64294
2008-03-21 23:00:26christian.heimessetmessages: + msg64292
2008-03-21 22:05:32gvanrossumsetmessages: + msg64287
2008-03-20 20:58:50christian.heimessetmessages: + msg64209
2008-03-20 20:50:17jimjjewettsetnosy: + jimjjewett
messages: + msg64207
2008-03-20 06:44:58gregory.p.smithsetnosy: + gregory.p.smith
messages: + msg64158
2008-03-20 02:15:15trentsetnosy: + trent
messages: + msg64137
2008-02-29 09:08:45christian.heimessetmessages: + msg63129
2008-02-29 08:45:11thervesetmessages: + msg63128
2008-02-24 13:49:10christian.heimessetfiles: - trunk_select_epoll_kqueue8.patch
2008-02-24 13:49:04christian.heimessetfiles: - trunk_select_epoll_kqueue6.patch
2008-02-24 13:49:00christian.heimessetfiles: - trunk_select_epoll_kqueue7.patch
2008-02-24 13:48:55christian.heimessetfiles: - trunk_select_epoll_kqueue5.patch
2008-02-24 13:48:42christian.heimessetfiles: + trunk_select_epoll_kqueue9.patch
messages: + msg62899
2008-02-04 15:11:47giampaolo.rodolasetnosy: + giampaolo.rodola
2008-01-20 10:16:46christian.heimessetfiles: + trunk_select_epoll_kqueue8.patch
2008-01-20 09:01:49christian.heimessetmessages: + msg60266
2008-01-19 19:53:18gvanrossumsetmessages: + msg60222
2008-01-19 14:45:25christian.heimessetmessages: + msg60176
2008-01-07 20:43:00gvanrossumsetmessages: + msg59487
2008-01-07 20:39:15christian.heimessetnosy: + gvanrossum
messages: + msg59486
components: + Extension Modules
2008-01-06 22:29:44adminsetkeywords: - py3k
versions: Python 2.6, Python 3.0
2007-12-25 17:13:17christian.heimessetfiles: + trunk_select_epoll_kqueue7.patch
messages: + msg58994
2007-12-24 10:50:04thervesetmessages: + msg58982
2007-12-23 21:07:29christian.heimessetfiles: + trunk_select_epoll_kqueue6.patch
messages: + msg58976
2007-12-22 16:18:36thervesetmessages: + msg58962
2007-12-22 11:09:05christian.heimessetmessages: + msg58961
2007-12-21 16:28:59thervesetmessages: + msg58947
2007-12-21 16:19:07thervesetfiles: + test_kqueue.diff
2007-12-21 13:07:37christian.heimessetfiles: - trunk_select_epoll_kqueue4.patch
2007-12-21 13:07:29christian.heimessetfiles: + trunk_select_epoll_kqueue5.patch
messages: + msg58944
2007-12-21 10:36:27thervesetmessages: + msg58941
2007-12-21 09:41:48christian.heimessetfiles: + trunk_select_epoll_kqueue4.patch
messages: + msg58938
2007-12-21 09:41:02christian.heimessetfiles: - trunk_select_epoll_kqueue3.patch
2007-12-21 09:40:58christian.heimessetfiles: - trunk_select_epoll_kqueue2.patch
2007-12-21 09:40:53christian.heimessetfiles: - trunk_select_epoll_kqueue.patch
2007-12-20 22:40:48christian.heimessetfiles: + trunk_select_epoll_kqueue3.patch
messages: + msg58929
2007-12-20 19:58:38christian.heimessetmessages: + msg58920
2007-12-20 18:34:20thervesetmessages: + msg58914
2007-12-20 14:44:00christian.heimessetfiles: + trunk_select_epoll_kqueue2.patch
messages: + msg58878
2007-12-20 12:44:55christian.heimessetfiles: + trunk_select_epoll_kqueue.patch
2007-12-20 12:44:41christian.heimessetfiles: - trunk_select_epoll_kqueue.patch
2007-12-20 12:44:37christian.heimessetfiles: - trunk_select_epoll3.patch
2007-12-20 10:25:59christian.heimessetfiles: + trunk_select_epoll_kqueue.patch
messages: + msg58867
title: [patch] select.epoll wrapper for Linux 2.6 epoll() -> [patch] epoll and kqueue wrappers for the select module
2007-12-19 17:45:02christian.heimessetfiles: + trunk_select_epoll3.patch
messages: + msg58814
2007-12-19 17:44:15christian.heimessetfiles: - trunk_select_epoll2.patch
2007-12-19 17:44:11christian.heimessetmessages: + msg58813
2007-12-19 17:43:16christian.heimessetmessages: - msg58795
2007-12-19 17:43:12christian.heimessetfiles: - trunk_select_epoll.patch
2007-12-19 10:50:49christian.heimessetmessages: + msg58804
2007-12-19 09:47:30thervesetnosy: + therve
messages: + msg58801
2007-12-19 09:41:03christian.heimessetfiles: + trunk_select_epoll2.patch
messages: + msg58800
2007-12-19 06:58:35christian.heimeslinkissue1675118 superseder
2007-12-19 06:57:08christian.heimescreate