Opened 4 years ago

Closed 3 years ago

#4862 defect closed fixed (fixed)

SerialPort (win32) incompatible with gtk2reactor

Reported by: detly Owned by: exarkun
Priority: normal Milestone:
Component: core Keywords:
Cc: jason.heeris@…, itamar@… Branch: branches/win32-gtk-win32events-4862
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

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.

Attachments (1)

tsgprob.py (428 bytes) - added by detly 4 years ago.

Download all attachments as: .zip

Change History (23)

Changed 4 years ago by detly

comment:1 Changed 4 years ago by michaelnt

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

comment:2 Changed 4 years ago by exarkun

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.

comment:3 Changed 4 years ago by detly

  • Cc jason.heeris@… added

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

comment:4 Changed 4 years ago by <automation>

comment:5 Changed 3 years ago by exarkun

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.

comment:6 Changed 3 years ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:7 Changed 3 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/win32-gtk-win32events-4862

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

comment:8 Changed 3 years ago by exarkun

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

refs #4862

comment:9 Changed 3 years ago by exarkun

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

comment:10 Changed 3 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted
  • Status changed from assigned to new

Build results. Please review.

comment:11 Changed 3 years ago by itamar

  • Cc itamar@… added
  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.

comment:12 Changed 3 years ago by exarkun

(In [32098]) Address the easy review points

refs #4862

comment:13 Changed 3 years ago by exarkun

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

refs #4862

comment:14 Changed 3 years ago by exarkun

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

refs #4862

comment:15 Changed 3 years ago by exarkun

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

refs #4862

comment:16 Changed 3 years ago by exarkun

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?

comment:17 Changed 3 years ago by itamar

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:

  1. Remove my name as maintainer from modules you've edited :)
  2. Extra line break before _ThreadedWin32EventsMixin.
  3. 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.)

comment:18 Changed 3 years ago by exarkun

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.

comment:19 Changed 3 years ago by itamar

  • Owner set to exarkun

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.

comment:20 Changed 3 years ago by itamar

  • Keywords review removed

Oops, and review is done.

comment:21 Changed 3 years ago by exarkun

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!

comment:22 Changed 3 years ago by exarkun

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

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

Note: See TracTickets for help on using tickets.