Opened 6 years ago

Closed 2 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, itamar@… Branch: branches/new-epoll-3114
(diff, github, buildbot, log)
Author: therve Launchpad Bug:

Description

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: http://pypi.python.org/pypi/select26. 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 6 years ago.
3114-updated.diff (146.1 KB) - added by MostAwesomeDude 2 years ago.
Updated #3114 patch for Twisted 12.0 trunk

Download all attachments as: .zip

Change History (23)

comment:1 Changed 6 years ago by therve

  • Milestone set to Python-2.6

Changed 6 years ago by therve

comment:2 Changed 6 years ago by therve

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

comment:3 Changed 6 years ago by therve

  • Owner changed from glyph to therve

comment:4 Changed 6 years ago by exarkun

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 6 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 5 years ago by therve

Apparently the kqueue bindins are buggy (http://bugs.python.org/issue5910)... Maybe latest 2.6 release will include the fix.

comment:7 Changed 4 years ago by glyph

  • Cc glyph added

comment:8 Changed 4 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 4 years ago by exarkun

  • Summary changed from Create reactors based on epoll and kqueue bindings present in Python 2.6 to Create reactor based on epoll bindings present in Python 2.6

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

comment:10 Changed 4 years ago by thijs

  • Cc thijs 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 4 years ago by glyph

  • Milestone Python-2.6 deleted

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 4 years ago by exarkun

#4645 was a duplicate of this.

comment:13 Changed 4 years ago by itamar

  • Cc itamar@… added

#4645 pointed to a patch:
http://code.google.com/p/pts-mini-gpl/source/browse/trunk/patches/pts-twisted-10.1.0-builtin-epoll.svn.diff

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 4 years ago by pts

Sure, feel free to use http://code.google.com/p/pts-mini-gpl/source/browse/trunk/patches/pts-twisted-10.1.0-builtin-epoll.svn.diff under "the MIT license", i.e. the contents of Twisted's LICENSE file.

comment:15 Changed 3 years ago by <automation>

  • Owner therve deleted

Changed 2 years ago by MostAwesomeDude

Updated #3114 patch for Twisted 12.0 trunk

comment:16 Changed 2 years ago by MostAwesomeDude

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

Thoughts?

comment:17 Changed 2 years ago by exarkun

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 2 years ago by therve

  • Author set to therve
  • Branch set to branches/new-epoll-3114

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

comment:19 Changed 2 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: buildbot.twistedmatrix.com/boxes-supported?branch=/branches/new-epoll-3114

comment:20 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

One of the unit tests failed on that buildbot run:

===============================================================================
[FAIL]
Traceback (most recent call last):
  File "/var/lib/buildbot/bot-glyph-1/lucid32-python2.6-select/Twisted/twisted/test/test_epoll.py", 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)

twisted.test.test_epoll.EPoll.test_badCreate
-------------------------------------------------------------------------------

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 2 years ago by therve

  • Resolution set to fixed
  • Status changed from new to closed

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