open() bug in RFC2217Serial...

[jszakmeister]

The bug is in the RFC2217Serial class's open method:

        self._thread = threading.Thread(target=self._telnetReadLoop)
        self._thread.setDaemon(True)
        self._thread.setName('pySerial RFC 2217 reader thread for %s' % (self._port,))
        self._thread.start()

        # negotiate Telnet/RFC 2217 -> send initial requests
        for option in self._telnet_options:
            if option.state is REQUESTED:
                self.telnetSendOption(option.send_yes, option.option)
        # now wait until important options are negotiated
        timeout_time = time.time() + self._network_timeout
        while time.time() < timeout_time:
            time.sleep(0.05)    # prevent 100% CPU load
            if sum(o.active for o in mandadory_options) == sum(o.state != INACTIVE for o in mandadory_options):
                break
        else:
            raise SerialException("Remote does not seem to support RFC2217 or BINARY mode %r" % mandadory_options)
        if self.logger:
            self.logger.info("Negotiated options: %s" % self._telnet_options)

The problem is when negotiations fail for some reason, and the SerialException is raised. If we make it to this point, the read thread is kept running in the background and the socket remains open as well. In our case, the RFC2217-capable device only allows one active connection at a time, so we can no longer re-establish our connection.

There are a few fixes that could be made. The first, and probably simplest, would be to make the setting of self._isOpen to True to before the creation of the read thread. That way if negotiations fail, the close() method can do its thing and tear the connection and the thread down.

The second option would be to close the socket and teardown the thread, then raise the SerialException. Since it's so close to wait the close() method does, it seems a bit redundant to take this approach.

A third option would be to change the close() implementation to something like this:

def close(self):
    """Close port"""
    wait = False
    if self._socket:
        try:
            self._socket.shutdown(socket.SHUT_RDWR)
            self._socket.close()
        except:
            # ignore errors.
            pass
        self._socket = None
        wait = True
    if self._thread and self._thread.is_alive():
        self._thread.join()
    self._isOpen = False

    # in case of quick reconnects, give the server some time
    if wait:
        time.sleep(0.3)

Then it's no longer keying on _isOpen to know whether to tear everything down. It looks at the actual resources and tears them down as necessary.

I prefer the first solution or the last solution, but thought I'd ask before filing a pull request.

[jszakmeister]

BTW, the same problem exists in PySerial 3.0.

[zsquareplusc]

i have checked in a fix in pySerial 3.x but had not yet time to test it properly.

I guess i would not fix that in the older releases. The new pySerial should be mostly backwards compatible so that the older ones are no longer needed.

[jszakmeister]

If you don't mind, I'd really like to see a 2.7.x released. 3.x is still pretty new, and the internals have had quite a bit of churn. For my project, PySerial is being used in a number of places, and we simply can't take on that risk right now. When 3.x hits stable and we get some cycles to test it thoroughly, then we can make that switch. Thanks!

[jszakmeister]

So I take it there will be no 2.7.x release with this fix? :-(