Opened 9 years ago

Closed 8 years ago

#1235 defect closed fixed (fixed)

Change wxreactor to use threadedselectreactor

Reported by: exarkun Owned by:
Priority: high Milestone:
Component: core Keywords: review
Cc: glyph, moonfallen, radix, exarkun, itamarst, etrepum, alecu, moof Branch:
Author: Launchpad Bug:

Attachments (1)

wxdemo.txt (2.6 KB) - added by itamarst 9 years ago.

Download all attachments as: .zip

Change History (32)

comment:1 Changed 9 years ago by exarkun

The public API for wxreactor stays the same as the rest of the reactors, but is
implemented in terms of threadedselectreactor's interleave method plus wx's
MainLoop.

Also, rename threadedselectreactor so that it is private, since it exposes the
wrong API for a reactor.

Also, figure out if Bob cares about cfreactor.  If not, deprecate/remove it.

Also, figure out if Bob's Cocoa example can be re-implemented without
non-framework calls to interleave().  If so, and if we remove the current
cfreactor, make a cfreactor that internally relies on threadedselectreactor
(just like wxreactor).

comment:2 Changed 9 years ago by etrepum

Kill cfreactor.  The cocoa examples are trivially refactored to use 
threadedselectreactor (if they aren't already).

Note that threadedselectreactor does have the reactor API also, but it's not at 
all interesting since using it as one ends up with a two threaded selectreactor.

Don't make a cfreactor that internally relies on threadedselectreactor.  
cfreactor *needs* the interleave API because Twisted *can not* start a Cocoa 
app.  cfreactor's run method actually was non-blocking in most situations, like 
interleave.

comment:3 Changed 9 years ago by glyph

Added #1241 (assigned to him) to reflect bob's concerns

comment:4 Changed 9 years ago by itamarst

It's not clear to me why the proposed tsr-based cfreactor reactor.run() can't
call the cf start loop command and have interleave() called in the appropriate
place (as wxreactor does now and will when it uses threadedselectreactor).
Wouldn't that have the same effect as users doing that manually?

comment:5 Changed 9 years ago by etrepum

Because you don't call a start loop command, the loop is already running by the 
time you have control.

comment:6 Changed 9 years ago by itamarst

Some issues with tsr: 
1. reactor shutdown events don't work at all without extra event loop specific work
2. the thread uses setDaemon(True), bleh

I have fixed these for my soon to be added tsr-using wxreactor.

comment:7 Changed 9 years ago by itamarst

in the new wxreactor the reactor.run() calls wxapp.MainLoop(), couldn't a
cfreactor run() call the appropriate CF function?

comment:8 Changed 9 years ago by etrepum

Not really.  It just can't work like that, tsr exists *because* the traditional 
twisted-on-the-outside approach is not appropriate in this case (and others).

comment:9 Changed 9 years ago by glyph

Let's just get the wx issues resolved first, before we drag CF into it.  CF
seems to have a much more involved mainloop situation.

comment:10 Changed 9 years ago by itamarst

The re-implemented wxreactor I did using tsr is in /branches/itamar/branch-1235
- someone should take a look and merge over the weekend. If you can get it to
run tests that would be even better; at the very least you'd need to have a top
frame registered or something or the mainloop exits when all windows exit.
SetExitOnFramesClose(False) or whatever that API is called might prevent that
though, you could try that in the DummyApp that gets installed if the user doesn't.

comment:11 Changed 9 years ago by radix

Well, given that 1. nobody seems to be happy with this 2. tsr seems to be in a
nasty state, and 3. wx has been broken forever, I don't think I'm going to
consider this release-blocking.

comment:12 Changed 9 years ago by itamarst

I do think we should merge this for 2.1, the whole point of this is to keep
people from using the current tsr, by making wxreactor use it behind the scenes.
Actually that bit is already done, this just makes wxreactor-using-tsr suck less.

So, wxreactor-using-tsr is *already* in 2.1. By merging this we'll make it work
better.

comment:13 Changed 9 years ago by itamarst

Oh, I see you released it. Now I have to write an email to the list sayig "the
correct way to use wx in 2.1 is using wxreactor but btw it's also has broken
signal handling and shutdown support." Can we have a 2.1.1? :)

comment:14 Changed 9 years ago by itamarst

Actually, my apologies, releasing without this was the right thing to do. I will
email the list a summary of our current status and what needs doing (by us and
by users) on Monday.

comment:15 Changed 9 years ago by moof

Tested on windows XP SP2, python 2.4.1, wxpython 2.6.1.0:

* File/Close Closes
* The Close Box Closes
* Alt+F4 Closes
* Alt f x Closes
* Ctrl-C from the command console closes
* Ctrl-C with the wx window in focus does not close (which is, I assume,
correct, so you can get copy and pasteworkign normally)
* Ctrl-Z from the command console *Does Not Close.* I am unsure as to whether
this is a bug or not. It's *supposed* to send some sort of signal to python, but
whether python catches it as a KeyboardInterrupt is a different matter.
* Sending and End Task message from the Task Manager Applications tab closes
gracefully (shows the shutting down... message)
* Terminating the python process from the processes tab kills python without
shutting down gracefully. This is correct.

comment:16 Changed 9 years ago by moof

I forgot to add:

Hello World keeps printing throughout the use of the menus and dialog box.

comment:17 Changed 9 years ago by moonfallen

Tested on mac os X.3.

"hello, world" kept scrolling while the dialog was open.  Alt-Q and File>Exit
both trigger (2,1,0) output in the console.  Unfortunately ^C in the console
causes "^C" to appear in the console, and the program exits immediately.

comment:18 Changed 9 years ago by alecu

Tested on Windows XP SP2, python 2.4.1 with wxPython2.6.1.0 then with wxPython
2.5.3.1:
Everything ok, as reported by moof

Tested on Mandrake 10.1, python 2.4, wxPythonGTK 2.5.3.1:
Everything works fine, but closing the window, both with Alt-f4 or the wm close
button does not work ok. It seems to hang up the reactor, because I see in the
log "shutting down in 0.3 seconds" immediately, but it takes a few seconds to
see "2..", "1...", "0..." (they show up together) then it hangs up and I need to
killall -1 python

comment:19 Changed 9 years ago by itamarst

I tested on Ubuntu with wxPython/wxWidgets 2.5.3 and it worked... so I'm not
sure what the problem with Mandrake is.

comment:20 Changed 9 years ago by itamarst

Another bug report: wxPython 2.4 has no wxEventLoop.

comment:21 Changed 9 years ago by moonfallen

I for one wouldn't care if wx 2.6 was required.

Changed 9 years ago by itamarst

comment:22 Changed 9 years ago by itamarst

Comment and attachment from another tester, via email:

--------------------------
Hello,
 
I have tested the svn version of wxreactor.py and doc/core/example/wxdemo.py
with Twisted 2.1. No problem with Windows (XP SP2) and Linux (Fedora Core 4)
both using wxPython 2.6.1.0.
 
However there are problems with Mac (OS X 10.4.3) using wxPython 2.6.1.0.
I have attached the trace I get on the Mac when closing wxdemo.py with the close
button of the window manager.
When I quit with CTRL+C in the terminal and the Quit option of the menu, I get
the same trace except that 2 1 and 0 are displayed before the error. "Hello
world" is not printed continously when I move the mouse in the menu or when I
click ok in the dialog without releasing it.
 
Please let me know if I can be of any further help. Thank you.
 
Maxime Vernier

comment:23 Changed 8 years ago by exarkun

  • Component set to conch

Merged this branch forward to threaded-wxreactor-1235

comment:24 Changed 8 years ago by exarkun

  • Component changed from conch to core

comment:25 Changed 8 years ago by itamarst

  • Keywords review added
  • Owner changed from itamarst to exarkun

comment:26 Changed 8 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to itamarst

Tested on Linux and Windows and OS X. Linux and Windows seem to work well with this version. OS X has all manner of problems, but they are probably all due to bugs in wxPython/Widgets that we can't do anything about. This also isn't any worse than the previous version on OS X, so there's no reason to block the Linux/Win32 improvements on this.

There should be a list of acceptance tests for wx, since we don't have unit tests for this. wxdemo.py currently serves as the basis for this - it should be split into two pieces, one for acceptance testing and one which is actually a demo for people who want to know how to use wx with Twisted and leaves out the various things which are primarily useful for determining whether or not wxreactor works. For the acceptance version, change the output so it is more obvious when events are not being processed.

Do some whitespace cleanup on whatever files were changed and need it. Remove the unnecessary change near the SystemExit handling in _threadedselect.py. Also add docstrings to all the methods in wxreactor.py.

comment:27 Changed 8 years ago by exarkun

Further investigation reveals a wx.Timer with a Notify method which calls ProcessPendingEvents on the App instance seems to cause Twisted events to be processed properly on OS X. Adding this to wxreactor (when running on OS X) would seem to make it work completely on all three platforms.

comment:28 Changed 8 years ago by itamarst

  • Description modified (diff)
  • Keywords review added
  • Owner changed from itamarst to exarkun

comment:29 Changed 8 years ago by itamarst

  • Description modified (diff)

Review comments have been addressed on a branch; please test new OS X fix. I won't have time to merge this week since I'm out of town, so I'd appreciate it if someone else would merge before 2.5 if it passes review.

comment:30 Changed 8 years ago by exarkun

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

(In [19070]) Merge wxreactor-1235+1574+1688

Author: itamar
Reviewer: exarkun
Fixes #1235
Fixes #1574
Fixes #1688

This fixes numerous problems with wxreactor:

  • startup and shutdown events are now supported
  • signal handlers will not be installed if installSignalHandlers=False is passed to run
  • a manual acceptance test has been added so the level of workingness of wxreactor can be examined

This also fixes the problem where modal GUI actions (displaying menus,
modal dialogs, resizing windows, etc) would block Twisted events from
being processed. On Linux, Windows, and OS X Twisted events will now
be serviced in these cases.

Additionally, _threadedselect no longer creates daemon threads and
the wxdemo has been simplified by the removal of code which was only
in support of acceptance testing (which is now a separate program).

comment:31 Changed 4 years ago by <automation>

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