UPDATE:* Better API with register(), unregister() and modify() instead of control()* Some documentation

Cool, thanks for working on that. Just for the record, I don't reallyunderstand the workflow: why closing the other ticket as duplicate andnot  post the patch on the old one? But whatever.For this patch, I don't see the benefit of putting it in the selectmodule, instead of a separate module. Is there a specific reason?Looking at the code, I don't have many remarks. pyepoll_new may leak ifepoll_create fails. I think that allowing threading around epoll_ctl isuseless, but I may be wrong.Thanks again for working on it.

> 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 withPyrex, ctypes or C. All of them have a slightly different API. I don'twant to break 3rd party software.The select module already contains an interface to select and poll. IMOit'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.

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 newmodule likehttp://bugs.python.org/issue1675118 and it wraps the epollcontrol 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 theconstants are all named "select.EPOLL_SPAM" instead of "SPAM".Missing:Documentation

Added license header to test_epoll.Some C code cleanups

First draft of a kqueue wrapper loosely based on Twisted's _kqueue.cfrom the kqreactor branch

UPDATE:* misc cleanups* added missing constants, see man kqueue(2)

Some remarks: * the name of the function used for PyArg_ParseTupleAndKeywords inregister, 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 begood to unify this (to tabs I guess, since the select module used tabsbefore). * it seems there is an unrelated change in sunau.py * I don't think the stdlib unittest module has skip support. You haveto 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 runthe whole twisted tests with it, so I don't think there are outstandingproblems. The code is fairly simple anyway.That's it for epoll and general remarks. I'll look at kqueue asap. Thanks!

> 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 thecase and the goto lables easier to read.>  * it seems there is an unrelated change in sunau.pyFixed>  * 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 problemslocally. I've also fixed a problem in unregister when fd is alreadyclosed and I'm using PyFile_AsFileDescriptor(). It supports ints andobjects with a fileno() method.I'm waiting for your test of kqueue before I upload the patch.

Here is the promised patch. I've added a small optimization. The objectsnow keep a buffer to reduce the malloc overhead.

I've fixed a bug with the preallocated buffer and in the repr() of kevent.

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 liketest_control_and_wait in test_epoll. * the kqueue_queue_control (and the pyepoll_poll) are now completelywrong! You should not limit to FD_SETSIZE, these 2 systems are therebecause they're able to handle for fds than that. Also, this bufferthing looks like a premature optimization. I'm unable to tell if it'scorrect or not. * the NETDEV and related flags aren't defined under OS X 10.4. I guessthere 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 thefunctionality is OK. But thsi FD_SETSIZE limit is a huge problem.

>  * the docstring of test_kqueue.py is wrongFixed>  * 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 lightweighted but I don't have spare time to write more tests. I may havemore 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 specsseveral times and I think you are right. I now use FD_SETSIZE assizehint 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. Iguess 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'sworking 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 richcompareslot to kevent. Do we need an alternative constructor which creates anepoll and kqueue object from a given fd?

I attached a patch with a more complete test of kqueue. It's not thatgreat, 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 beusefull in some corner cases, but that's not a priority.exarkun looked at the patch and told me that there may be somethreadsafety issues: for example, when calling epoll_wait, you useself->evs unprotected. It's not very important, but you may want to tellit in the documentation.As you started the rich comparison for kevent objects, it may beinteresting to have full comparison (to sort list of events). It's not ahigh priority though.That's all for now!

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

> What do you suggest as sort criteria?The natural sort of the tuple you used for equality, I'd say.

I had some trouble with your patch. BSD doesn't raise a socket errorwith EINPROGRESS and it sets the ENABLE and ADD flags to 0.The new patch removes the preallocated buffer and adds fromfd() classmethods. kevent objects are fully orderable, too.

You have to use sys.platform to get 'darwin', not os.name. The rest ofthe test seems good.I didn't spot the check of EEXIST in pyepoll_internal_ctl, I'm not sureit'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 atest 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_DELETEMaybe 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 begreat to have that in python :).

UPDATE: * Removed EEXIST magic * timeout is now milliseconds but supports float for smaller timeouts * poll object has a modify method, too

Guido, have you reviewed the patch and are you fine with it?

Not yet, I ran out of time. Can you hold on for another week?

Can somebody else review the patch? therve from the Twisted team hasreviewed it but I like to get an opinion of another core developer.Guido seems to be too busy.

Still haven't had the time (sorry!), but one comment: please don'tspecify timeouts in millisecond. We use seconds (floats if necessary)everywhere else in Python, regardless of the underlying data structureor resolution.

Yeah, it's a reasonable suggestion. I'm changing the code to seconds aspositive float and None = blocking.

I've updated the patch. The latest patch didn't contain the unit testsand it failed to apply cleanly, too.

Is there a chance for this go in the first alpha? FWIW, I've tested itwith 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 docsinstead of KQ_EV_ADD. Everything else looks good to me.

I love to get it into the next alpha but I don't have time to today. Canyou take it to the mailing list and ask somebody to review and submitthe patch?

Patch applies cleanly on FreeBSD 6.2-STABLE and all tests pass.  Can't comment on technical accuracy.

+1trunk_select_epoll_kqueue9.patch  looks good to me.style nit: I'd just use self.fail("error message") instead of raiseAssertionError("error message") within unittests.  regardless, both workso I don't care. :)

Is pyepoll a good prefix?  To me, it looks a lot like the _Py and Py reservered namespaces, but not quite...

I had to use some kind of prefix to avoid naming collisions with theepoll_* functions for the epoll header file. pyepoll sounded reasonableto me.

pyepoll for static names sounds fine (assuming you want some consistency). Given all the rave reviews, what are the chances that you'll be checkingthis in soon?

Say "Go" and I'll check the patch in ASAP.

Go.

I've applied the patch inr61722.TODO: finish the documentation, any help is appreciated

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 createdissue 5910 and included a patch, which also adds another test case. Anything else I need to do to get the patch accepted?

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?

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