win32 SerialPort requires a reactor that provides "addEvent" method, but doesn't have a good error message if it gets a different one

[tiendalinux]

SerialPort raises the following exception on Windows if it is used with the default reactor:

  File "twisted\internet\_win32serialport.py", line 56, in __init__
    self.reactor.addEvent(self._overlappedRead.hEvent, self, 'serialReadEvent')
AttributeError: 'SelectReactor' object has no attribute 'addEvent'

It should say something more helpful when given a SelectReactor, explaining that win32eventreactor is required for serial port support on Windows.

[Itamar Turner-Trauring]

On windows you need to use the win32eventreactor, e.g.:

from twisted.internet import win32eventreactor
win32eventreactor.install()

before you do any other imports, and all the rest of the code can be the same.

[Itamar Turner-Trauring]

In particular, you need to use this reactor for *serial* support, not for normal usage.

[Glyph]

There's a valid problem here, that's both a documentation and specification problem: twisted.internet.serialport's documentation is unclear as to what is required of the reactor parameter to SerialPort, and it differs between platforms. addEvent is not specified on any reactor interface.

To close this ticket I would say that we need an addEvent method that provides an error message that explains what itamar just did in response to this ticket. There may also be a need for a separate ticket for an interface that explains the semantics of addEvent; ideally this could be added to e.g. the IOCP reactor as well.

[Glyph]

Since this is a documentation issue, it should be pretty easy to resolve.

[tiendalinux]

Sorry, i initially didn't understand how to use the Win32Reactor, i finally was able to use it, this is a code example, that reads from the serial port and print everything on the screen:

#!/usr/bin/python2.4

from twisted.internet.win32eventreactor import Win32Reactor from twisted.internet import protocol from twisted.internet.serialport import SerialPort

class MyProtocol( protocol.Protocol ) :

def dataReceived(self, data):

print data

def connectionLost(self, reason='connectionDone'):

print reason

if name == "main":

port = 0 protocol = MyProtocol() reactor = Win32Reactor() myserial = SerialPort (protocol, port, reactor) print "Server running, press ctrl-C to stop." reactor.run()

[Jean-Paul Calderone]

We should still improve our documentation.

[Lucas Taylor]

See comments below

[Lucas Taylor]

twisted-3802.patch does the following:

• Creates a new IReactorWin32Events interface w/ 2 win32-specific methods: addEvent & removeEvent
• win32eventreactor now also implements IReactorWin32Events
• _win32serialport.SerialPort asserts that the reactor passed in provides the IReactorWin32Events interface

[TimAllen]

Thanks for writing this patch!

A few code-review comments:

1. I notice the patch you supplied has a few lines with trailing whitespace, you might want to clean that up.
2. Twisted likes to keep lines to 79 columns max, so your import in twisted.internet.win32eventreactor and your error message in twisted.internet._win32serialport will need to be wrapped.
3. assert is a very big stick to wield. Raising something a little less violent like ValueError would be better.
4. I think your error message is a bit verbose - it's quite possible that some other reactor will work, other than win32eventreactor, and the reactor-installation instructions are well-covered in other parts of the documentation, so no need to repeat them here. Something like this should be sufficient:

   SerialPort on Windows requires a reactor that implements IReactorWin32Events, such as win32eventreactor.
   

5. I think people investigating Twisted's serial support would best be served by having some actual documentation in twisted.internet.serialport. In an ideal world, there would be an ISerialPort interface describing all the common functionality of the POSIX, Win32 and Java SerialPort implementations, with full docstrings and all that stuff. At the very least, however, twisted.internet.serialport should have a module docstring that mentions that:
   • Twisted's serial port support requires pyserial (perhaps linking to its homepage)
   • On Windows, you need to use a reactor that supports IReactorWin32Events.
6. There should be a unittest in twisted.internet.test.test_win32eventreactor asserting that win32eventreactor implements IReactorWin32Events (zope.interface.verify.verifyClass should come in handy here). That test file doesn't exist yet, so you'll probably want to read about adding new modules in the ​Coding Standard. Since win32eventreactor imports Win32-specific modules, you'll need prevent the tests from running on other platforms. I can't find a good example of this in the code at the moment, but I imagine it would look something like this:

   from twisted.trial import unittest
   from twisted.python.runtime import platform
   
   if platform.isWindows():
       from twisted.internet import win32eventreactor
   else:
       win32eventreactor = None
   
   class TestWin32EventReactor(unittest.TestCase):
   
       if win32eventreactor is None:
   	skip = "Win32EventReactor tests can only run on Windows"
   
       # ...actual test methods...
   

Again, thank you for tackling this. Better error messages make life more pleasant for everybody involved!

[Lucas Taylor]

Thanks for the review.

re: 5 A docstring added to twisted.internet.serialport is easy enough.

I may work on an Interface, but it would probably be good to modify the implementations to actually use it. Should that go in a separate ticket?

[TimAllen]

If it's up to me, I say this ticket is for making a helpful error message on Win32, and re-factoring the code to use interfaces would be a separate ticket.

[Lucas Taylor]

Attached patch should address the first review

1. Cleaned up trailing whitepace
2. Wrapped imports @ 79 chars
3. ValueError is raised if user instantiates a SerialPort instance on Windows w/o installing a correct reactor
4. Simplified error message in ValueError
5. Added docstring to twisted.internet.serialport re: requirements
6. Added test: test_win32eventreactor to verify it provides the IReactorWin32Events interface

I think this covers the error msg and doc issue and provides a base for fleshing out IReactorWin32Events and tests elsewhere. ISerialPort will have to wait for another day/ticket.

[amaury]

In the IReactorWin32Events interface, the description of @param action is inaccurate: the actual code passes a string, and the action is run as getattr(fd, action)().

@param action is not a callable, but the name of a method.

[Lucas Taylor]

Thanks for the correction. Modified as follows:

@param action: a string that is a method name of the fd instance.
               This method is called in response to the event.

[Lucas Taylor]

Patch addresses review

[Jean-Paul Calderone]

(In [29190]) Branching to 'win32er-serialport-3802'

[Jean-Paul Calderone]

(In [29191]) Apply twisted-3802.2.patch

refs #3802

[Jean-Paul Calderone]

(In [29192]) Add the test module as well

refs #3802

[Jean-Paul Calderone]

Thanks. This mostly seems good. We should probably document the new exception that SerialPort can raise. I think we also need to write the tests in the reactormixins.ReactorBuilder style, because that way our existing Windows build slaves can run the new test. We don't actually have anything doing trial -r win32er ... at the moment.

[Jean-Paul Calderone]

Did that. ​Build results

[Lucas Taylor]

SerialPort docstring

[Lucas Taylor]

Replying to exarkun:

We should probably document the new exception that SerialPort can raise.

We should probably document SerialPort in general ;)

I've attached a docstring patch, but there's an issue in that the signatures for _posixserialport and _win32serialport differ for some reason (@param timeout). Fixing _win32serialport would reorder the parameters...

I think we also need to write the tests in the reactormixins.ReactorBuilder style, because that way our existing Windows build slaves can run the new test. We don't actually have anything doing trial -r win32er ... at the moment.

I'm pretty sure I don't understand... doesn't buildbot just run everything in the test packages? What is the benefit of reactormixins.ReactorBuilder esp. considering how simple test_win32eventreactor is?

Thanks for the review.

[Jean-Paul Calderone]

(In [29421]) Apply 3802_docstring.patch

refs #3802

[Jean-Paul Calderone]

(In [29423]) Branching to 'win32er-serialport-3802-2'

[Jean-Paul Calderone]

I'm pretty sure I don't understand... doesn't buildbot just run everything in the test packages?

Yep. But "trial twisted" runs all of the tests using the default reactor - currently select on all platforms. So tests written in the obvious style, using the global reactor, get run against the select reactor, not the win32 event reactor. In the past we've dealt with this using the trial --reactor option, configuring slaves that run "trial --reactor win32er twisted" alongside slaves that use the default reactor or other reactors. However, it's been a long time since we had a slave configured to actually do "trial --reactor win32er twisted".

The reactormixins.ReactorBuilder style of writing tests generates a test case for each reactor at runtime, so that even when doing "trial twisted", the tests are still run against every available reactor. This is how we prefer to write new tests, since it simplifies the buildbot configuration (and simplifies running the tests in general).

For the rest of this, I'm going to do some refactoring to reduce some code duplication between posix and windows implementations. Thanks also for pointing out the mixed up parameter order. I'm pretty sure the correct fix here is to make the windows order match the posix order, since these are supposed to be implementations of a single interface, and posix was the better working and more widely used of the two implementations.

[Jean-Paul Calderone]

To make it easier to find reviews, I split the code in this branch up into #4523 and #4525. Once those are resolved, we can update this branch to just include the new Windows serialport interface checking, which will make this branch much shorter and focused on this issue.