Opened 12 years ago

Closed 11 years ago

#1760 enhancement closed fixed (fixed)

Rewrite iocpreactor using code taken from twisted.internet.{tcp|abstract|udp}

Reported by: PenguinOfDoom Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: Jean-Paul Calderone, ghazel, itamarst Branch: branches/iocpreactor-1760-6
branch-diff, diff-cov, branch-cov, buildbot
Author: PenguinOfDoom

Description

By carefully monkeypatching select, socket or doSelect (or parameterizing), one could reuse most of twisted.internet for alternative concurrency mechanisms.

Attachments (1)

1760.zip (2.2 KB) - added by ghazel 12 years ago.

Download all attachments as: .zip

Change History (15)

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

I think you typed "monkeypatching" when you meant to type "refactoring".

comment:2 Changed 12 years ago by PenguinOfDoom

Summary: Rewrite iocpreactor to be a small add-on to selectreactorRewrite iocpreactor using code taken from twisted.internet.{tcp|abstract|udp}

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

Cc: Jean-Paul Calderone ghazel added

How's this going?

Have we done any testing of real applications with this code yet? Is it ready for that?

comment:4 Changed 12 years ago by ghazel

I gave iocpreactor-1760-3 a spin, and ran in to a few issues:

  • build.bat specifies mingw32. I took a stab and removed that in favor of msvc7.1 which python was built with
  • it couldn't find python24.lib. I specified c:\python24\libs\ in library_dirs and it compiled
  • t.i.iocp.reactor._doIteration fails when timeout is None (pretty often). Pulling a selectreactor and doing "if timeout is None: timeout = 0.01" got that running
  • memory usage is an order of magnitude (21mB vs 119mB with 800 local to local connections) better than iocp1 or 2, but still not as small as selectreactor (15mB) or my growing buffersize patch for iocp1 (15mB). This is livable, I just thought I'd mention it.
  • using pause/resumeProducing produced this error:
Traceback (most recent call last):
  File "twisted\python\log.py", line 48, in callWithLogger
    return callWithContext({"system": lp}, func, *args, **kw)
  File "twisted\python\log.py", line 33, in callWithContext
    return context.call({ILogContext: newCtx}, func, *args, **kw)
  File "twisted\python\context.py", line 59, in callWithContext
    return self.currentContext().callWithContext(ctx, func, *args, **kw)
  File "twisted\python\context.py", line 37, in callWithContext
    return func(*args,**kw)
--- <exception caught here> ---
  File "C:\Documents and Settings\user\Desktop\projects\twisted\Twisted\branches
\iocpreactor-1760-3\twisted\internet\iocpreactor\reactor.py", line 106, in _call
EventCallback
    evt.callback(rc, bytes, evt)
  File "twisted\internet\iocpreactor\abstract.py", line 70, in _cbRead
    if self._handleRead(rc, bytes, evt):
  File "twisted\internet\iocpreactor\abstract.py", line 84, in _handleRead
    assert self._readSize == 0, str(self._readSize) # you're welcome
exceptions.AssertionError: 4096

just before that, _dispatchData returned from inside the "while self._readNextBuffer < full_buffers" loop, presumably because producing was paused during the loop.

  • python24.dll often crashes just after that, with an access violation.

comment:5 Changed 12 years ago by PenguinOfDoom

  • Don't use build.bat. It's not for you. Use setup.py in the branch root.
  • distutils finds Python headers and libraries automatically. Perhaps your install is broken.
  • In what situation is doIteration called with timeout of None?
  • Perhaps it might be worthwhile to decrease FileHandle.readBufferSize to 1024 and bump FileHandle.maxReadBuffers to 64, or whatever. I prefer to waste a small amount of memory per connection and avoid constantly allocating and deallocating arbitrarily-sized buffers.
  • I would greatly appreciate a minimal example that produces this behavior.

Changed 12 years ago by ghazel

Attachment: 1760.zip added

comment:6 Changed 12 years ago by ghazel

doIteration is called with a timeout of None in this simple example:

from twisted.internet import iocpreactor
iocpreactor.install()
from twisted.internet import reactor
reactor.run()

I added 1760.zip, an example that reproduces the producer problem. Basically, it's an example that forms 5 connections, and spews data across them. The download side has a rate limiter, and because of all that pause/resume producing the bug is displayed.

comment:7 Changed 12 years ago by itamarst

Cc: itamarst added

comment:8 Changed 11 years ago by Jean-Paul Calderone

I merged this branch forward and resolved some conflicts in it. Some of the non-iocp changes should probably be broken out into separate tickets.

The code is in iocpreactor-1760-4 for the moment, though. Note that svn 1.4 is required to merge this branch correctly.

comment:9 Changed 11 years ago by pahan

author: pahan
Branch: branches/iocpreactor-1760-5

(In [22833]) Branching to 'iocpreactor-1760-5'

comment:10 Changed 11 years ago by pahan

Branch: branches/iocpreactor-1760-5branches/iocpreactor-1760-6

(In [22838]) Branching to 'iocpreactor-1760-6'

comment:11 Changed 11 years ago by PenguinOfDoom

author: pahanPenguinOfDoom
Keywords: review added
Owner: changed from PenguinOfDoom to therve

It passes all tests

comment:12 Changed 11 years ago by therve

Keywords: review removed
Owner: changed from therve to PenguinOfDoom
  • reactor.py uses a set(), it should use a twisted.python.compat.set object
  • the way the reactor attribute of connection objects is managed is wrong. It should be done at the lowest level by FileHandle, like what FileDescriptor is doing.
  • the default timeout of 0 in doIterate in wrong I think. 1 is probably a more reasonable default value. If you just call reactor.run(), it eats all the CPU.

I can probably nitpicking forever, but I think it's good now. So do these little things, run the tests suite, and then please merge.

Thanks.

comment:13 Changed 11 years ago by pahan

Resolution: fixed
Status: newclosed

(In [22957]) Merge iocpreactor-1760-6: new iocpreactor implementation

This is new and improved iocpreactor that passes all tests.

Author: PenguinOfDoom Reviewer: therve Fixes #1760

comment:14 Changed 8 years ago by <automation>

Owner: PenguinOfDoom deleted
Note: See TracTickets for help on using tickets.