Package for testing network connection

Question

I wrote this package for monitoring the state of a network connection in the terminal because my net connection would go down for a few minutes at a time and come back up. I think it could be used in other applications.

I'm looking for any advice on how to improve or extend this package in any way possible.

Package repo

My concerns are as follows:

1. Is the package "Pythonic"?
2. Does the package contain any obvious bugs/errors/flaws?

#!/usr/bin/env python


'''
This module is for checking a networks status.
'''

import socket
from time import sleep as _sleep
from functools import wraps
import errno
import os
import signal

TIMEOUT = 2

class NetworkStatus(object):
    def timeout(seconds=1, error_message=os.strerror(errno.ETIME)):
        def decorator(func):
            def _handle_timeout(signum, frame):
                return
                signal.signal(signal.SIGALRM, _handle_timeout)
                signal.alarm(seconds)
            def wrapper(*args, **kwargs):
                try:
                    result = func(*args, **kwargs)
                finally:
                    signal.alarm(0)
                return result

            return wraps(func)(wrapper)

        return decorator


    def set_timeout(self, t_out):
        '''
        Update the global variable TIMEOUT.
        '''
        global TIMEOUT
        TIMEOUT = t_out

    def sleep(self,delay):
        '''
        Sleep function with cleanup.
        '''
        try:
            _sleep(delay)
        except KeyboardInterrupt:
            exit()


    @timeout(TIMEOUT)
    def check(self, **kw):
        '''
        Check if the network is up.
        '''
        self.sleep(TIMEOUT)
        try:
            socket.gethostbyname(kw.get('remote_server', 'www.google.com'))
            return True
        except Exception:
            return False


    def online(self, **kw):
        '''
        return True is the network is UP.
        '''
        return self.check(**kw) is True


    def offline(self, **kw):
        '''
        return False if the network is DOWN.
        '''
        return self.online(**kw) is False


    def if_online(self, func, *args, **kw):
        if self.online():
            func(*args, **kw)

    def if_offline(self, func, *args, **kw):
        if self.offline():
            func(*args,**kw)


    def mainloop(self):
        '''
        Continuously check if the network is UP or DOWN.
        '''
        while True:
            up = self.check()
            if up:
                print 'Network is up'
            else:
                print 'network is down.'


if __name__ == '__main__':
    NetworkStatus().mainloop()

Answer 1

Importing sleep as _sleep is an awkward work around. It'd be much easier to do a plain import time, then in your sleep function refer to time.sleep. No need for odd aliases and nothing gets overwritten.

This function does nothing:

def _handle_timeout(signum, frame):
    return
    signal.signal(signal.SIGALRM, _handle_timeout)
    signal.alarm(seconds)

It will immediately return, and the following two lines will never be executed.

If TIMEOUT can be set then it's not a constant, and it should instead be a class level variable. Place it directly after the class definition, then you can directly assign it easily without needing to resort to the global keyword. You also don't need to pass it as an argument to @timeout, you can just use NetworkStatus.timeout directly there.

class NetworkStatus(object):

    timeout = 2

In online you're returning if self.online(**kw) is True, but this is the same as returning self.online(**kw) already is True. And check only returns true or false. There's no reason to have two separate functions here at all, just rename the vague check function to is_online and you're returning a boolean directly from there. Then you also have offline which makes little sense. If you're concerned about readability, then if not network.is_online() is plenty readable without having a specific function.

Further down you also have if_online and if_offline... what are these wrappers for? They seem overly generic to achieve one small goal (only run a function if the network is online/offline. It'd make more sense to have these specific to the function, rather than passing the function as an argument. Did you have a lot of functions that needed this construct? It doesn't look like you've used it here.

I'd rename mainloop to just run, or run_test. mainloop is a generic name that tells us nothing. Also, no need to store the result in up. You can evaluate directly:

if self.is_online():

Now that I'm here I notice that you never passing any keyword arguments as kw**. Indeed you never use them, instead just using string literals:

socket.gethostbyname(kw.get('remote_server', 'www.google.com'))        

This is very bad practice! Those values should be constants or class attributes. You should also remove all the unnecessary kw** parameters. Why let the user pass in values that'll never be used for anything?

Answer 2

Your code seems to have a mild sleep disorder.

---

Why does this wrapper exist?

def sleep(self,delay):
    '''
    Sleep function with cleanup.
    '''
    try:
        _sleep(delay)
    except KeyboardInterrupt:
        exit()

If you had just done import time, then you could call time.sleep(delay) instead of the weirdly named _sleep().

Why do you catch KeyboardInterrupt there? Why should the program behave differently if you press CtrlC while it is sleeping, compared to while it is in some other phase of the loop? To establish a uniform policy for handling keyboard interrupts, just move the KeyboardInterrupt exception handler to the main loop.

Once you've mad both of these changes, there is no longer any reason to write your own sleep() function at all. I'd go further and say that online(), offline(), if_online(), and if_offline() are all superfluous, since I don't see how they could be useful. After ripping out those functions, it looks more like a simple function is all you need.

---

The way you call self.sleep(TIMEOUT) from check() is not right either.

I'd expect check() to do just that: perform the test immediately. Not after some delay. If you need to throttle the test, then the sleep call belongs in the loop, not the test.

You're using TIMEOUT for both the test frequency and the maximum allowable response time. Why should the they be equal? I think I would want to perform the test maybe once a minute, and allow five seconds for a response. If you want to monitor the connection continuously, then it seems that ICMP pings would be more appropriate than DNS lookup tests.