SerialPort (win32) incompatible with gtk2reactor

[Jason]

When an attempt is made to make a serial port connection under the gtk2reactor in Windows, the following traceback results:

File "C:\Python26\lib\site-packages\twisted\internet\_win32serialport.py", line 56, in __init__
    self.reactor.addEvent(self._overlappedRead.hEvent, self, 'serialReadEvent')
exceptions.AttributeError: 'PortableGtkReactor' object has no attribute 'addEvent'

The attached code take a port name as the first argument. It can be run under Linux, and doesn't do anything visible. Under Windows, you get the traceback.

[Michael]

I think this is a duplicate of #3802, provide a better error message for reactors that don't support addEvent.

[Jean-Paul Calderone]

I think detly wants PortableGtkReactor to actually implement IReactorWin32Events. It's certainly not too cool that you can't use gtk and serial ports together on Windows with Twisted right now.

[Jason]

Just noticed #3802 which is related, but exarkun is spot on in that I *can't* use the Win32 reactor, since it's a GTK app.

Back story: I was using a mess of threads and locking to get PyGTK and PySerial working together consistently under Windows and Linux. At the emphatic advice of #python, and much assistance from #twisted, I re-wrote my app to use Twisted instead. Since my dev environment is Linux, I didn't notice I'd sacrificed the ability to use the app on Windows — the main deployment environment in our workplace!

tiny violin starts playing...

[Jean-Paul Calderone]

So, the obvious fix is for PortableGtkReactor to implement IReactorWin32Events. It would do that by calling MsgWaitForMultipleObjects in a separate thread, I guess. Since PortableGtkReactor is just a thinly disguised SelectReactor it doesn't seem like it would be easy to get Windows event notification in the same thread.

[Jean-Paul Calderone]

(In [32008]) Branching to 'win32-gtk-win32events-4862'

[Jean-Paul Calderone]

(In [32009]) implement IReactorWin32Events for SelectReactor by giving it a Win32Reactor

refs #4862

[Jean-Paul Calderone]

This seems to be largely complete in the branch. However, there's some stickiness with isInIOThread which I haven't entirely sorted out yet.

[Jean-Paul Calderone]

​Build results. Please review.

[Itamar Turner-Trauring]

1. twisted/internet/selectreactor.py:18: 'IReactorWin32Events' imported but unused
2. win32eventreactor._ThreadFDWrapper: Can we assume logPrefix is always thread-safe? (It *probably* is).
3. win32eventreactor._ThreadFDWrapper._execute: you should probably be using blockingCallFromThread, otherwise you're losing the remove-fd-if-action-handler-throws-exception behavior. That would mean connectionLost would be run in the thread, though, so you'd need to fix that too. Or you can recreate the exception handling in _execute, somehow.
4. Add a test making sure that when the threaded win32 reactor shuts down it will *not* call connectionLost on anything (if there isn't one already, haven't gotten to tests yet.)
5. The docstring for _ThreadFDWrapper should indicate the reactor it has is the non-win32 reactor, for clarity.

More comments later.

[Jean-Paul Calderone]

(In [32098]) Address the easy review points

refs #4862

[Jean-Paul Calderone]

(In [32099]) Avoid calling the logPrefix method in the wrong thread

refs #4862

[Jean-Paul Calderone]

(In [32100]) Dispatch the connectionLost callback to the main reactor thread

refs #4862

[Jean-Paul Calderone]

(In [32101]) Also pass on the handler's return value, in cause it is meant to cause the event source to disconnect.

refs #4862

[Jean-Paul Calderone]

Thanks very much. Great points about the threaded interaction for logPrefix and connectionLost. I think I addressed the issue, it wasn't so bad. I just called logPrefix in the main thread and passed it along, rather than calling it each time in the secondary thread. For exceptions raised from the application callback, blockingCallFromThread indeed gets the secondary reactor to disconnect the handler, and a suitable callFromThread-using connectionLost method on _ThreadFDWrapper ensures that application code can't tell there's an extra thread involved.

I didn't address point 4 yet. There's something to be done there, I think, but I don't think we want to prevent connectionLost from being called - I think we want to ensure it is called at the right time?

[Itamar Turner-Trauring]

Re #4: Currently, AFAICT registering an event with win32eventreactor does *not* result in connectionLost on shutdown; that is only tied to read/write registration. connectionLost *will* be called from main reactor. So, there's nothing to do in actual code. However, we want to make sure this continues to be the case, thus my suggestion to add a test.

Changes look good. More issues, besides above:

6. Remove my name as maintainer from modules you've edited :)
7. Extra line break before _ThreadedWin32EventsMixin.
8. If I set _registerAsIOThread to False in epoll, and then run "trial twisted.test.test_threadpool twisted.internet.test.test_threads", the test_isInIOThread incorrectly passes... because trial's reactor has already registered in the thread. Global state, it sucks.

More tomorrow, Jenny wants to go to sleep (sorry for the disjointed review.)

[Jean-Paul Calderone]

Re #4: Currently, AFAICT registering an event with win32eventreactor does *not* result in connectionLost on shutdown; that is only tied to read/write registration. connectionLost *will* be called from main reactor. So, there's nothing to do in actual code. However, we want to make sure this continues to be the case, thus my suggestion to add a test.

Wow, terrible. :/ I guess I can add a test for that (r32108), it seems like stupid behavior though.

Remove my name as maintainer from modules you've edited :)

r32107

Extra line break before _ThreadedWin32EventsMixin.

Looks like 3 blank lines to me, that's right according to the coding standard.

if I set _registerAsIOThread to False in epoll, and then run "trial twisted.test.test_threadpool twisted.internet.test.test_threads" ... Global state, it sucks.

Bleh, indeed. I'm inclined to deprecate isInIOThread and, say, add a new reactor method. Separate ticket, though.

[Itamar Turner-Trauring]

Re r32108, it's stupid behavior, yes, but mostly I was getting at we needed to make sure connectionLost wasn't called multiple times or in wrong thread.

Separate ticket for #8, and making it reactor method, seems like the way to go.

One remaining worry: if you forget to implement(Win32EventsReactor) in the right place, the relevant tests won't run and no one will notice that anything broke. Perhaps I'm worrying too much, though, so probably not necessary to do anything.

So unless you care about that: looks good, ready to merge.

[Itamar Turner-Trauring]

Oops, and review is done.

[Jean-Paul Calderone]

Separate ticket for #8, and making it reactor method, seems like the way to go.

Filed #5153, #5154, and #5155.

One remaining worry: if you forget to implement(Win32EventsReactor) in the right place, the relevant tests won't run and no one will notice that anything broke.

True. Something like this happened to cfreactor recently (and has not yet been fixed, I think). This is a problem with the skip features of the ReactorBuilder-style tests though. And I don't really know how to describe the problem (at least not in more detail than "sometimes stuff gets skipped that shouldn't get skipped") nor how we might fix it. It's not a new problem with this branch, at least, so I'm happy merging this without addressing that problem. I did manually verify that the all the tests that I want to be running are running before I submitted this for review, at least. :)

So unless you care about that: looks good, ready to merge.

Yay, thanks!

[Jean-Paul Calderone]

(In [32109]) Merge win32-gtk-win32events-4862

Author: exarkun Reviewer: itamarst Fixes: #4862

Add IReactorWin32Events to the select, iocp, and Gtk reactors on Windows (if the third-party dependencies are available) by having each of those reactors run a Win32Reactor in a secondary thread and delegating event- related methods to that reactor.

Significantly, this means all reactors on Windows now support serial ports.