Opened 7 years ago
Closed 7 years ago
#4862 defect closed fixed (fixed)
SerialPort (win32) incompatible with gtk2reactor
Reported by: | Jason | Owned by: | Jean-Paul Calderone |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | core | Keywords: | |
Cc: | Jason, itamarst | Branch: |
branches/win32-gtk-win32events-4862
branch-diff, diff-cov, branch-cov, buildbot |
Author: | exarkun |
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)
Change History (23)
Changed 7 years ago by
Attachment: | tsgprob.py added |
---|
comment:1 Changed 7 years ago by
comment:2 Changed 7 years ago by
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 7 years ago by
Cc: | Jason 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 7 years ago by
comment:5 Changed 7 years ago by
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 7 years ago by
Owner: | set to Jean-Paul Calderone |
---|---|
Status: | new → assigned |
comment:7 Changed 7 years ago by
Author: | → exarkun |
---|---|
Branch: | → branches/win32-gtk-win32events-4862 |
(In [32008]) Branching to 'win32-gtk-win32events-4862'
comment:8 Changed 7 years ago by
comment:9 Changed 7 years ago by
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 7 years ago by
Keywords: | review added |
---|---|
Owner: | Jean-Paul Calderone deleted |
Status: | assigned → new |
Build results. Please review.
comment:11 Changed 7 years ago by
Cc: | itamarst added |
---|
- twisted/internet/selectreactor.py:18: 'IReactorWin32Events' imported but unused
- win32eventreactor._ThreadFDWrapper: Can we assume logPrefix is always thread-safe? (It *probably* is).
- 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.
- 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.)
- The docstring for _ThreadFDWrapper should indicate the reactor it has is the non-win32 reactor, for clarity.
More comments later.
comment:13 Changed 7 years ago by
comment:14 Changed 7 years ago by
comment:15 Changed 7 years ago by
comment:16 Changed 7 years ago by
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 7 years ago by
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:
- Remove my name as maintainer from modules you've edited :)
- Extra line break before _ThreadedWin32EventsMixin.
- 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 7 years ago by
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 :)
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 7 years ago by
Owner: | set to Jean-Paul Calderone |
---|
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:21 Changed 7 years ago by
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 7 years ago by
Resolution: | → fixed |
---|---|
Status: | new → 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.
I think this is a duplicate of #3802, provide a better error message for reactors that don't support addEvent.