epoll and kqueue wrappers for the select module

[tiran]

BPO	1657
Nosy	@gvanrossum, @akuchling, @terryjreedy, @gpshead, @giampaolo, @tiran, @tpn, @intgr
Files	test_kqueue.diff trunk_select_epoll_kqueue9.patch

^{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:

assignee = 'https://github.com/tiran'
closed_at = <Date 2010-07-10.23:43:25.091>
created_at = <Date 2007-12-19.06:57:08.176>
labels = ['type-feature', 'docs']
title = 'epoll and kqueue wrappers for the select module'
updated_at = <Date 2010-07-10.23:43:25.087>
user = 'https://github.com/tiran'

bugs.python.org fields:

activity = <Date 2010-07-10.23:43:25.087>
actor = 'terry.reedy'
assignee = 'christian.heimes'
closed = True
closed_date = <Date 2010-07-10.23:43:25.091>
closer = 'terry.reedy'
components = ['Documentation']
creation = <Date 2007-12-19.06:57:08.176>
creator = 'christian.heimes'
dependencies = []
files = ['9020', '9535']
hgrepos = []
issue_num = 1657
keywords = []
message_count = 38.0
messages = ['58800', '58801', '58804', '58813', '58814', '58867', '58878', '58914', '58920', '58929', '58938', '58941', '58944', '58947', '58961', '58962', '58976', '58982', '58994', '59486', '59487', '60176', '60222', '60266', '62899', '63128', '63129', '64137', '64158', '64207', '64209', '64287', '64292', '64294', '64299', '89588', '99763', '109945']
nosy_count = 11.0
nosy_names = ['gvanrossum', 'akuchling', 'terry.reedy', 'gregory.p.smith', 'jimjjewett', 'therve', 'giampaolo.rodola', 'christian.heimes', 'trent', 'intgr', 'Erik Gorset']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue1657'
versions = ['Python 2.6', 'Python 3.1', 'Python 2.7', 'Python 3.2']

[tiran]

UPDATE:

• Better API with register(), unregister() and modify() instead of control()
• Some documentation

[therve]

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.

[tiran]

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.

[tiran]

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

[tiran]

Added license header to test_epoll.
Some C code cleanups

[tiran]

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

[tiran]

UPDATE:

• misc cleanups
• added missing constants, see man kqueue(2)

[therve]

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!

[tiran]

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.

[tiran]

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

[tiran]

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

[therve]

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.

[tiran]

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

[therve]

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!

[tiran]

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?

[therve]

What do you suggest as sort criteria?

The natural sort of the tuple you used for equality, I'd say.

[tiran]

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.

[therve]

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 :).

[tiran]

UPDATE:

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

[tiran]

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

[gvanrossum]

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

[tiran]

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.

[gvanrossum]

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.

[tiran]

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

[tiran]

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

[therve]

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.

[tiran]

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?

[tpn]

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

[gpshead]

+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. :)

[jimjjewett]

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

[tiran]

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.

[gvanrossum]

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?

[tiran]

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

[gvanrossum]

Go.

[tiran]

I've applied the patch in r61722.

TODO: finish the documentation, any help is appreciated

[ErikGorset]

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

[akuchling]

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?

[terryjreedy]

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