wadda / gps3

Python 2.7 - 3.5 interface to gpsd
MIT License
76 stars 32 forks source link

sys.stderr.write error #2

Closed proximous closed 8 years ago

proximous commented 8 years ago

When GPSD is not running, it should throw ConnectionRefusedError: [Errno 111] Connection refused, however gps3.py, line 70 has a bug:

sys.stderr.write('\nGPSDSocket.connect OSError is-->', error)
TypeError: write() takes exactly 1 argument (2 given)
proximous commented 8 years ago

I'm also a bit confused about the intended error management. I'd like to write code that handles a failed connection, however the sys.exit(1) prevents me from managing a connection failure myself. Is there an intended way to enable me to handle connection failures?

proximous commented 8 years ago

I should clarify that I am using Python3. The behavior of Python2.7 is different because in Python2.7, the socket failure is an IOError which is not caught by the OSError on line 69. So I guess with version 0.20.1 I can catch the TypeError in Python3 or the IOError in Python2 and manage this myself, however I'd still like to see this fixed along with a proper way for me to manage connection failures. Thanks!

wadda commented 8 years ago

Yes, there is the intended way of receiving notification and rolling off intermittent failures back into a functional state, while fatal errors would give reason it's fatal. It just hasn't gotten there.

The bug was there. Thank you. The exception was modified

            except (OSError, IOError) as error:
                sys.stderr.write('\nGPSDSocket.connect exception is--> {}'.format(error))

Subsequently, if there is no sys.exit(1) following an [Errno 113] No route to host or [Errno 111] Connection refused error GPSDSocket.send hammers a BrokenPipeError: [Errno 32] Broken pipe.

self.close() could be called to close the dead-end(113) or refused(111) socket, and clean up, but masks those errors behind a BrokenPipeError.

I don't want to kick the can down the road, as the problem is Errno 113 or Errno 111, and not Errno 32 but, I am in a quandary and I'm looking for a more elegant solution.

If you have a preference, I'm all ears.

proximous commented 8 years ago

Thanks for the quick reply! Personally, I don't ever want a library calling sys.exit(). IMHO, sys.exit() should only be used in the __main__of an application.

I'd prefer to see gps3.GPSDSocket() throw an exception when it fails to connect, and let the application catch the exception and act accordingly. You can self.close() if that makes sense prior to throwing the exception.

For me, I'm currently catching both TypeError (thrown by the sys.stderr.write in Python3) and the IOError (thrown by the socket failure in Python2.7). I need to be able to handle edge/race conditions where, for example, my application starts before GPSD is running, or where GPSD is restarted. I also found I needed to implement a timeout to handle GPSD restarts.

My application uses a thread that looks like this:

    def connect(self):
        try:
            self.connection = gps3.GPSDSocket(self.host, self.port, self.protocol, self.device)
            self.gps_fix = gps3.Fix()
            self.idle_failure = time.time() + self.idle_timeout
        except (TypeError, IOError) as err:
            # GPS3 Version 0.20.1 throws a TypeError in Python3 and IOError in Python2.7
            self.connection = None
            time.sleep(1)

    def disconnect(self):
        if isinstance(self.connection, gps3.GPSDSocket):
            self.logger_GPS.info("DISCONNECTING")
            self.connection.close()
            self.connection = None

    def run(self):
        self.running = True
        while self.running:
            if self.connection is None:
                self.connect()
            else:
                try:
                    for new_data in self.connection:
                        if not self.running:
                            break
                        if new_data:
                            self.idle_failure = time.time() + self.idle_timeout
                            self.gps_fix.refresh(new_data)
                            self.publish_data()
                        elif time.time() > self.idle_failure:
                            self.disconnect()
                            break
                        # Set sleep to close to update rate to minimize CPU load
                        time.sleep(self.sleep_time)
                except Exception as err:
                    self.logger_GPS("{}: {}".format(type(err).__name__, err))
        self.disconnect()

    def stop(self):
        self.running = False

This currently passes my test cases so I'm happy with it.

wadda commented 8 years ago

It looks nice.

I've been thinking what what you said, and rather embarrass myself thinking aloud I need to test some ideas. Thanks for your input.

wadda commented 8 years ago

The functions were unhooked from the daisy-chain that required a sys.exit. Now one must explicitly call for an 'connect' and a 'watch'. 'watch' still "send"s the command as I cannot envision a situation where the former would be without the latter.