Opened 9 years ago

Closed 5 years ago

#3114 enhancement closed fixed (fixed)

Create reactor based on epoll bindings present in Python 2.6

Reported by: therve Owned by: therve
Priority: normal Milestone:
Component: core Keywords:
Cc: Glyph, Thijs Triemstra, itamarst Branch: branches/new-epoll-3114
branch-diff, diff-cov, branch-cov, buildbot
Author: therve


This is meta-ticket to list the task neededs to integrate the new bindings. Python 2.6 introduces 2 new bindings to epoll and kqueue, which can be used to create a reactor. Porting epoll is not really urgent for us, except it removes the need of our own bindings. We're not sure that pyrex will work in 2.6.

The kqueue work should probably happen in #1918.

A separate module has been released by Christian Heimes here: It needs to be updated to match the one in Python itself. The license is MIT, so we may want to distribute it with Twisted (but that could cause some other problems, so I'm not sure we want to do this).

Attachments (2)

epoll.diff (5.7 KB) - added by therve 9 years ago.
3114-updated.diff (146.1 KB) - added by Corbin Simpson 5 years ago.
Updated #3114 patch for Twisted 12.0 trunk

Download all attachments as: .zip

Change History (23)

comment:1 Changed 9 years ago by therve

Milestone: Python-2.6

Changed 9 years ago by therve

Attachment: epoll.diff added

comment:2 Changed 9 years ago by therve

For the record, I attached a patch on epollreactor using the new module.

comment:3 Changed 9 years ago by therve

Owner: changed from Glyph to therve

comment:4 Changed 9 years ago by Jean-Paul Calderone

Would it be sensible to instead provide a compatibility module between our epoll interface and the new stdlib epoll interface? The changes seem superficial. Then epollreactor won't have to change, it can just use the stdlib functionality when it's present, ours otherwise.

comment:5 Changed 9 years ago by therve

I would prefer to change our epoll interface to match the one of stdlib. At some point, we will able to remove it, whereas if we add another layer, epollreactor will use an interface that doesn't make sense anymore.

comment:6 Changed 8 years ago by therve

Apparently the kqueue bindins are buggy ( Maybe latest 2.6 release will include the fix.

comment:7 Changed 7 years ago by Glyph

Cc: Glyph added

comment:8 Changed 7 years ago by Glyph

This is related to #1918; maybe even a duplicate of it? Except that one doesn't mention epoll. "and" in a ticket summary is always a bad sign :-\.

comment:9 Changed 7 years ago by Jean-Paul Calderone

Summary: Create reactors based on epoll and kqueue bindings present in Python 2.6Create reactor based on epoll bindings present in Python 2.6

Okay, so let's just make this about epoll.

comment:10 Changed 7 years ago by Thijs Triemstra

Cc: Thijs Triemstra added

Perhaps we should move this ticket to the 2.7 milestone and close the 2.6 milestone? This is currently the last open ticket for the 2.6 milestone, and 2.7 has been released.

comment:11 Changed 7 years ago by Glyph

Milestone: Python-2.6

Actually this shouldn't be in a Python-X.Y milestone at all. Our milestones don't have descriptions, but my understanding of the Python-X.Y milestones is "make Twisted fully work on that version of Python". This is just an enhancement which requires a particular python version; we don't need it to make Twisted work right on 2.6 or 2.7.

comment:12 Changed 6 years ago by Jean-Paul Calderone

#4645 was a duplicate of this.

comment:13 Changed 6 years ago by Itamar Turner-Trauring

Cc: itamarst added

#4645 pointed to a patch:

If it's under GPL (as indicated on main page for project) we can't use it though, so if we're interested in this specific patch then pts will need to relicense it.

comment:14 Changed 6 years ago by Péter Szabó

Sure, feel free to use under "the MIT license", i.e. the contents of Twisted's LICENSE file.

comment:15 Changed 6 years ago by <automation>

Owner: therve deleted

Changed 5 years ago by Corbin Simpson

Attachment: 3114-updated.diff added

Updated #3114 patch for Twisted 12.0 trunk

comment:16 Changed 5 years ago by Corbin Simpson

Uploaded a version of the patch which cleanly applies against 12.0-ish trunk.

There are some real issues in here; the patch expands what twisted.python._epoll can do to match socket.epoll, and modifies EpollReactor to match. However, this causes some tests to regress. Ideally, we'd do this in three chunks:

  • Update twisted.python._epoll to have the same API as socket.epoll (and deprecate _epoll's old API?)
  • Update EpollReactor to use the new API, fix regressing tests
  • Add the ImportError switch to try socket.epoll first, fix regressing tests


comment:17 Changed 5 years ago by Jean-Paul Calderone

What tests fail?

The old _epoll API does not need to be deprecated. It's a private API, we can do anything we want to it without regard for backwards compatibility. Since only a very small, specific portion of Twisted uses the existing API, I think it's pretty safe for us to just change both it and the one consumer of it.

comment:18 Changed 5 years ago by therve

Author: therve
Branch: branches/new-epoll-3114

(In [33571]) Branching to 'new-epoll-3114'

comment:19 Changed 5 years ago by therve

Keywords: review added

It looks mostly good to me. I removed the suspicious time.sleep call (because it can't be interrupted), and I didn't see test failures. I suspect the wakeup FD is always there so it's never empty. Otherwise, we would need to pass -1 when the list is empty.

Build results are there:

comment:20 Changed 5 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to therve

One of the unit tests failed on that buildbot run:

Traceback (most recent call last):
  File "/var/lib/buildbot/bot-glyph-1/lucid32-python2.6-select/Twisted/twisted/test/", line 83, in test_badCreate
    self.assertRaises(TypeError, _epoll.epoll)
twisted.trial.unittest.FailTest: TypeError not raised (<twisted.python._epoll.epoll object at 0x301be58> returned)


But I guess you fixed it afterwards.

Otherwise, all the tests seem to pass everywhere, and I guess the interfaces line up, so that's good. A comment was deleted from doPoll, it might be good to restore that.

The other thing that might be nice is to not bother trying to build or install our custom epoll bindings if Python has select.epoll. That would be fine as a follow-up ticket, though.

Otherwise, +1. Please merge after addressing that one point.

comment:21 Changed 5 years ago by therve

Resolution: fixed
Status: newclosed

(In [33579]) Merge new-epoll-3114

Authors: pts, MostAwesomeDude, therve Reviewer: exarkun Fixes: #3114

Use the epoll bindings included in Python standard library when available instead of our custom twisted.python._epoll. It also changes the custom ones to match the standard library API.

Note: See TracTickets for help on using tickets.