7
\$\begingroup\$

I wrote this class that checks for internet connectivity on a background thread. Targeting Python 3.10.

It has two main design goals:

  • Minimize the time get_state() (gets connectivity state) takes to run.
  • Be polite to the test server (8.8.8.8 in this case): Stop hitting it if nothing appears interested in the connectivity state.

The way it behaves is it periodically checks for connectivity on another thread, providing access to the connection state via get_state(). If get_state() is not called for some configurable amount of time, however, then the background thread stops checking for connectivity. If get_state() is called while it's in this idle mode, get_state() will knock it back out of idle mode and wait for the first connectivity check to complete. This is the only case where get_state() might take "a long time", which is fine.

The context here is this is a background thread running in a back-end HTTP server, where the front-end (running on the same machine) polls it for internet connectivity via an HTTP endpoint (that calls get_state()). The reason "politeness" is important is that it doesn't make sense to keep trying 8.8.8.8 if the user isn't using the front-end, which is about 99% of the time in this case. It's on a single-user device so only one person will be infrequently interacting with this.

My two main concerns are:

  1. Are there any weird potential race conditions that I'm not seeing here?
  2. Can I simplify this at all? In particular, can I get rid of any of the condition vars?

The code is here and it seems to behave as designed:

import logging
import socket
import time
import threading

from typing import Optional

logging.basicConfig(level=logging.DEBUG)
_logger = logging.getLogger()


class ConnectivityChecker:
    """Periodically checks internet connectivity in the background."""
    
    def __init__ (self, check_interval: float, idle_timeout: float):
        """Checks connection every check_interval seconds, stops checking if get_state()
        has not been called for idle_timeout seconds."""

        self._check_interval = check_interval
        self._idle_timeout = idle_timeout
        
        self._run = True
        self._state_cond = threading.Condition()  # for _state
        self._state = None
        self._rtime_cond = threading.Condition()  # for _rtime
        self._rtime = 0.0
        self._delay_cond = threading.Condition()  # for interruptible interval timer
        self._thread = threading.Thread(target=self._threadfunc)

        self._thread.start()

    def _threadfunc (self) -> None:

        _logger.info("Thread started.")

        while (self._run):

            # idle wait: wait for new get_state() call if too much time has passed
            # since the last one. state is set to None when we're idling. ends early
            # if thread is shutting down.
            with self._rtime_cond:
                while self._run and time.monotonic() - self._rtime > self._idle_timeout:
                    with self._state_cond:
                        self._state = None
                    self._rtime_cond.wait()
                if not self._run:
                    break

            # check connection; notify get_state() if it's waiting on a None state.
            state = self._is_connected()
            with self._state_cond:
                self._state = state
                self._state_cond.notify_all()

            # interval timer: pause for a bit before checking state again, end early
            # if thread is shutting down.
            with self._delay_cond:
                while self._run:
                    if not self._delay_cond.wait(self._check_interval):
                        break

        _logger.info("Thread stopped.")

    def _is_connected (self) -> bool:
        """Returns true if we're connected to the internet, false if we're probably not."""

        _logger.debug("Ping...")
        
        s = None
        try:
            s = socket.socket()
            s.settimeout(10)
            s.connect(("8.8.8.8", 53))
            return True
        except Exception as x:
            return False
        finally:
            if s is not None:
                s.close()

    def get_state (self) -> Optional[bool]:
        """Will return None only if the thread was shut down while waiting for state."""

        # notifies _threadfunc if it happens to be idle, and updates _rtime.
        with self._rtime_cond:
            self._rtime = time.monotonic()
            self._rtime_cond.notify_all()

        # if _state is None then thread was idle so wait for a result. otherwise this
        # will just immediately return the last result.
        with self._state_cond:
            while self._run and self._state is None:
                self._state_cond.wait()
            return self._state

    def shutdown (self) -> None:
        """Shut down thread."""

        if self._thread is not None:

            _logger.info("Shutting down thread...")
        
            # wake everybody up after setting _run to False so all threads see it.
            self._run = False
            with self._rtime_cond:
                self._rtime_cond.notify_all()
            with self._state_cond:
                self._state_cond.notify_all()
            with self._delay_cond:
                self._delay_cond.notify_all()

            self._thread.join()
            self._thread = None

The test code I used is this:

if __name__ == "__main__":

    c = ConnectivityChecker(2, 5)

    print("sleeping 10 seconds")
    time.sleep(10)

    print("checking five times in a row")
    print(c.get_state())
    print(c.get_state())
    print(c.get_state())
    print(c.get_state())
    print(c.get_state())

    print("sleeping 10 seconds")
    time.sleep(10)

    print("checking five times with 3 seconds between")
    for k in range(5):
        print(c.get_state())
        time.sleep(3)

    print("sleeping 10 seconds")
    time.sleep(10)
    
    c.shutdown()
\$\endgroup\$
2
  • \$\begingroup\$ If this code is to be used on a web server, and a client sends an HTTP request to the server , doesn't that already make the whole "check internet connectivity" pointless? I mean, if the server can receive the request and respond to it, we can probably say it has internet connectivity (unless running locally with the client, which seems odd). Wouldn't an echo/healthcheck endpoint on the server suffice? \$\endgroup\$ Commented Jun 23 at 20:14
  • \$\begingroup\$ @MiguelAlorda No, the front and back end run on the same machine. :) \$\endgroup\$
    – Jason C
    Commented Jun 23 at 20:31

2 Answers 2

3
\$\begingroup\$

There are some difficulties with the OP code. The two aspects are:

  1. design of service
  2. implementation of a given design

design

abuse of the digital commons

This is just not good. It discourages companies like Google from establishing or maintaining public servers which some clients will misuse for unrelated purposes. We wouldn't want to see a popular lib or app pick this up.

            s = socket.socket() ...
            s.connect(("8.8.8.8", 53))
            return True

That first line is specifying SOCK_STREAM, or TCP. So we're doing a {SYN, SYN-ACK, ACK} three-way handshake, asking server to allocate a PCB, then violating DNS protocol by sending zero bytes, then finishing with a RST. It's never a good idea to unconditionally increment someone else's error counters on every single transaction. And if the RST gets lost by the network, the PCB resource needs to stick around for a server-configured timeout interval. This should have been a single UDP query out with a UDP response back. Issue a simple "$ dig ns google.com" type of query. The async-dns package offers good control over the details.

config

The second line should lookup the protocol rather than hardcoding it. But it especially should accept a server FQDN, such as "ns4.google.com", perhaps from an env var or config file. Better still, it should simply accept a domain name, then query the NS set to choose an authoritative nameserver for that domain. Then anyone operating this code could easily point it at locally supported services that they pay for, like "myisp.net".

Pickle the result and use it after a restart, in case there's no net at that time.

use case

I assume the output of get_state() is used to render a "healthy!" icon, or perhaps to grey-out certain input fields. I would have found it helpful to see mention of such UX details in the Review Context.

implementation

specification

This service is fundamentally racy, and that's fine, everyone buys into it and accepts it. The instant after a port 53 packet is received, we might see powerfail or a CAT-5 cable unplugged. So any "internet connection is healthy!" claims can always suffer some amount of staleness. Recent health doesn't guarantee the next packet we send will be delivered. We only care about "good enough".

Alas, neither the Review Context nor the code pin down what that is, there's no window to bound staleness.

polling responsibility

Part of the trouble is that the timing of this library's queries is controlled by caller, and we don't impose a contract on caller so it's responsible for a certain poll rate. A simpler design would make caller responsible for saying "I am interested", and then have this library schedule the queries. We could use a bool to express interest, or perhaps a more robust design would be a Dead Man's Switch or Watchdog Timer, where each caller update extends the polling interval for perhaps a minute or so. Notice that such calls involve no delay or timeout behavior, they successfully return immediately.

datastructure

With isochronous polling scheduled, we can put proper bounds on staleness. Define a pair of timestamps variables:

  • sent
  • received

It may also be convenient to set a RTT statistic upon receiving each DNS reply. If outbound polling is active and we have a "recent" received stamp with a "small" rtt then our connection is known to be healthy, else it may be down. We can always report this to caller with zero interactive delay.

If you want to get fancy, you might also track recent {minimum, mean, SD} on the roundtrip time.

meaningful names

This is not a good name:

    def _threadfunc(self) -> None:

I do thank you for the type annotation, and would have welcomed e.g. : bool = True annotations elsewhere so I could rule out None.

Please call this method _poller or _connection_checker, since that's what it does. Please give it a """docstring""". You may wish to point out this is a thread function in that docstring.

        self._rtime = 0.0

Please give that a slightly longer name, perhaps _recv_time. This apparently initializes it to January 1970 boot time, which seems surprising. Consider giving the Gentle Reader a clue about its semantics by initializing it to the current time. If you don't rename it, then it at least deserves a # comment describing its purpose.

EDIT: @JasonC observes in the comments that, while monotonic() does advance by 1.0 every second, it does not start at the unix epoch.

many locks

As written the OP code does multiple waits, notifies, and mutex acquires over three(!) cond vars. There seems to be more complexity than necessary, largely induced by loose timing requirements for both caller and library. Using simpler isochronous polling with a pair of timestamps would make deadlock analysis much simpler. We have an opportunity to satisfy each caller request immediately, with network events happening on a background thread.

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Regarding 8.8.8.8 news.ycombinator.com/…. tends to agree with you. \$\endgroup\$
    – chicks
    Commented Jun 24 at 22:46
  • \$\begingroup\$ This is fantastic thank you. Lots to think about here. Btw, I'm using time.monotonic() for timestamps, which officially defines no reference point. But you're right, 0 is still technically inappropriate. The best I can do is initialize it to time.monotonic() - self._idle_timeout, or introduce a second variable to keep track of state and not rely on the initial value of _rtime. \$\endgroup\$
    – Jason C
    Commented Jun 24 at 22:55
  • \$\begingroup\$ And, heard re: 8.8.8.8. It would be especially bad if I accidentally got a user's home/office network blacklisted. Fwiw, after I posted this I did in fact make the test server address and port configurable and add fqdn support. \$\endgroup\$
    – Jason C
    Commented Jun 24 at 22:57
  • \$\begingroup\$ I've switched from making invalid DNS requests to doing a valid HTTP request to a server that we own. That part feels much better now at least. Thanks again, btw. \$\endgroup\$
    – Jason C
    Commented Jun 25 at 14:07
6
\$\begingroup\$

Why 3 threading.Condition instances?

You have defined 3 threading.Condition instances when I believe you could have simplified the code with one instance (which I will explain later). This makes answering your question "Are there any weird potential race conditions that I'm not seeing here?" more difficult to answer. Adding to the difficulty is that you have no comments describing what each instance represents or is used for.

A code simplification

Using a context manager provided by the socket.socket class, you can simplify method is_connected as follows:

    def _is_connected (self) -> bool:
        """Returns true if we're connected to the internet, false if we're probably not."""

        _logger.debug("Ping...")
        
        try:
            with socket.socket() as s:
                s.settimeout(10)
                s.connect(("8.8.8.8", 53))
        except Exception as x:
            return False
        else:
            return True

Issue with method get_state

What if a call to get_state is made after a call to shutdown has been made. Not likely, perhaps, but my thinking is that it would be better to model the state has having 3 possible values, which cannot be represented with a bool. So we might have instead:

State = Enum('State', ['CONNECTED', 'NOT_CONNECTED', 'SHUTDOWN'])

Then get_state can return State.SHUTDOWN if it is called after a shutdown has been performed. Naturally, method _is_connected would return a State enum instance.

Proposed changes

You have two threads interacting with one another, i.e. the main thread's get_status method and the child thread's worker function _threadfunc. It is clear that there are some critical sections where (1) each thread must have exclusive control over modifying the state of the ConnectivityChecker instance and (2) each thread must wait for certain conditions to be fulfilled by the other thread. I believe both requirements can be achieved with a single Condition instance, self._cond. Let's look at each function:

get_state needs to know that there is a "recent connection state" it can return to its caller. This means that it can immediately return the current state if the _threadfunc thread has not been "idling", i.e. not polling connection status because more than self._idle_timeout seconds has elapsed since the last call to get_state. Otherwise, we must assume that the _threadfunc thread has stopped its periodic checking of the connection state and is waiting for a new call to get_state to "wake it up" and resume polling the connection state. Then get_state must wait for that new connection state update to have occurred.

_threadfunc must be in a loop where it first gets the current connection status and then "sleeps" for self.__check_interval seconds before checking again. But before it checks again, it should see how long ago get_state was previously called. If this exceeds self._idle_time then it must go into its "idle" state until get_state is called again.

get_state and _threadfunc take turns notifying one another of some event that has occurred and then waiting for a an event to occur in response. I don't see why this cannot be handled by a single condition.

I update variable self._last_call_time, intially set to 0.0, every time get_status is called. _threadfunc will check this against the current time and if it exceeds self._idle_time it goes into a wait after setting self._idling = True. If get_status sees that _threadfunc is idling, it will wake it up and then wait for a new update to the connection status to occur. I increment instance variable self._update_count, initially set to 0, whenever _threadfunc` updates the connection status.

In the code below I have modified the socket address to be 127.0.0.1 for testing:

import logging
import socket
import time
import threading
from enum import Enum

logging.basicConfig(level=logging.DEBUG)
_logger = logging.getLogger()

State = Enum('State', ['CONNECTED', 'NOT_CONNECTED', 'SHUTDOWN'])

class ConnectivityChecker:
    """Periodically checks internet connectivity in the background."""

    def __init__ (self, check_interval: float, idle_timeout: float):
        """Checks connection every check_interval seconds, stops checking if get_state()
        has not been called for idle_timeout seconds."""

        self._check_interval = check_interval
        self._idle_timeout = idle_timeout

        self._run = True
        self._last_call_time = 0.0
        self._update_count = 0
        self._idling = False
        self._state = State.SHUTDOWN  # Arbitrary
        self._cond = threading.Condition()

        # Make this a daemon thread so that we can shutdown immediately
        # even if it is testing a connection:
        threading.Thread(target=self._threadfunc, daemon=True).start()

    def _threadfunc (self) -> None:
        """Periodically update our connection status but idle if
         get_state has not been called recently."""

        _logger.info("Thread started.")

        while self._run:
            with self._cond:
                # Before we test the connection, see how long ago
                # the last call to get_state was:
                if (self._last_call_time == 0.0 or
                time.monotonic() - self._last_call_time >= self._idle_timeout):
                    _logger.debug('Idling until the next call to get_state or shutdown')
                    self._idling = True
                    self._cond.wait()
                    self._idling = False

                self._state = self._test_connection()
                self._update_count += 1
                self._cond.notify_all()

                # Wait for next test or shutodwn
                self._cond.wait(self._check_interval)

                if not self._run:
                    break


        _logger.info("Thread stopped.")

    def _test_connection(self) -> State:
        """Set current state and time of last check."""

        _logger.debug("Ping...")

        try:
            with socket.socket() as s:
                s.settimeout(10)
                #s.connect(("8.8.8.8", 53))
                s.connect(("127.0.0.1", 80))
        except Exception as x:
            return State.NOT_CONNECTED
        else:
            return State.CONNECTED

    def get_state (self) -> State:
        """Return the current state."""

        with self._cond:
            if not self._run:
                return self._state

            # Set new last call time:
            self._last_call_time = time.monotonic()

            # Is the thread idling?
            if self._idling:
                self._cond.notify_all()
            elif self._update_count != 0:
                _logger.debug('Returning previously set state')
                return self._state

            # We must wait for an update of state:
            last_update_count = self._update_count
            time_to_wait = self._check_interval  # Or some other value
            expiration_t = time.monotonic() + time_to_wait  # We won't wait pass this future time
            while self._run and self._update_count == last_update_count and time_to_wait > 0.0:
                self._cond.wait(time_to_wait)
                # Compute next time to wait in case we must wait some more.
                time_to_wait = expiration_t - time.monotonic()

            return self._state

    def shutdown (self) -> None:
        _logger.info("Shutting down ...")
        self._state = State.SHUTDOWN
        self._run = False
        with self._cond:
            self._cond.notify_all()

if __name__ == "__main__":

    c = ConnectivityChecker(2, 5)

    print("sleeping 10 seconds")
    time.sleep(10)

    print("checking five times in a row")
    print(c.get_state())
    print(c.get_state())
    print(c.get_state())
    print(c.get_state())
    print(c.get_state())

    print("sleeping 10 seconds")
    time.sleep(10)

    print("checking five times with 3 seconds between")
    for k in range(5):
        print(c.get_state())
        time.sleep(3)

    print("sleeping 10 seconds")
    time.sleep(10)

    c.shutdown()
    print(c.get_state())

Prints:

INFO:root:Thread started.
sleeping 10 seconds
DEBUG:root:Idling until the next call to get_state or shutdown
checking five times in a row
DEBUG:root:Ping...
State.CONNECTED
DEBUG:root:Returning previously set state
State.CONNECTED
DEBUG:root:Returning previously set state
State.CONNECTED
DEBUG:root:Returning previously set state
State.CONNECTED
DEBUG:root:Returning previously set state
State.CONNECTED
sleeping 10 seconds
DEBUG:root:Ping...
DEBUG:root:Ping...
DEBUG:root:Idling until the next call to get_state or shutdown
checking five times with 3 seconds between
DEBUG:root:Ping...
State.CONNECTED
DEBUG:root:Ping...
DEBUG:root:Returning previously set state
State.CONNECTED
DEBUG:root:Ping...
DEBUG:root:Returning previously set state
State.CONNECTED
DEBUG:root:Ping...
DEBUG:root:Ping...
DEBUG:root:Returning previously set state
State.CONNECTED
DEBUG:root:Ping...
DEBUG:root:Returning previously set state
State.CONNECTED
DEBUG:root:Ping...
DEBUG:root:Ping...
sleeping 10 seconds
DEBUG:root:Ping...
DEBUG:root:Idling until the next call to get_state or shutdown
INFO:root:Shutting down ...
State.SHUTDOWN
DEBUG:root:Ping...

Update

In the following new version of get_state, the only "excessive" waiting could possibly occur the very first time get_state is called since prior to that call we have been "idling" and a connection state has not been polled yet.

    def get_state (self) -> State:
        """Return the current state."""

        if not self._run:
            return self._state

        # We only need to acquire the condition if we are currently
        # idling or if this is the very first call to get_state (in
        # which case we must wait for an initial status). Otherwise,
        # we just return the current status.

        # Set new last call time:
        self._last_call_time = time.monotonic()

        if self._idling:
            with self._cond:
                if self._idling:  # Check again to avoid needless notify
                    self._cond.notify_all()

        if self._update_count == 0:
            # We must wait for an initial status update.
            # This is the only case that could take a while.
            with self._cond:
                while self._run and self._update_count == 0:
                    self._cond.wait()
                self._last_call_time = time.monotonic()  # Update

        return self._state    

However, if we have been idling for a long time and then we call get_state, it will be returning a connection status that could be very stale. If instead you prefer to wait for a new status update before returning, then use the following:

    def get_state (self) -> State:
        """Return the current state."""

        if not self._run:
            return self._state

        # We only need to acquire the condition if we are currently
        # idling or if this is the very first call to get_state (in
        # which case we must wait for an initial status). Otherwise,
        # we just return the current status.

        # Set new last call time:
        self._last_call_time = time.monotonic()
        
        if self._idling or self._update_count == 0:
            with self._cond:
                if self._idling:  # Check again to avoid needless notify
                    self._cond.notify_all()
                last_update_count = self._update_count
                while self._run and self._update_count == last_update_count:
                    self._cond.wait()
                self._last_call_time = time.monotonic() # Update

        return self._state

Alternate Design

The following version:

  1. Does not hold any locks (condition instances) while it is actually testing a connection.
  2. Removes the necessity for instance variable self._update_count.
  3. Endeavors to support multiple threads calling get_state, event though this wasn't explicitly mentioned as a requirement.
import logging
import socket
import time
import threading
from enum import Enum

logging.basicConfig(level=logging.DEBUG)
_logger = logging.getLogger()

State = Enum('State', ['CONNECTED', 'NOT_CONNECTED', 'SHUTDOWN'])

class ConnectivityChecker:
    """Periodically checks internet connectivity in the background."""

    def __init__(self, check_interval: float, idle_timeout: float):
        """Checks connection every check_interval seconds, stops checking if get_state()
        has not been called for idle_timeout seconds."""

        self._check_interval = check_interval
        self._idle_timeout = idle_timeout

        self._run = True
        self._start_time = self._last_call_time = time.monotonic()
        self._idling = True  # We must be initially idling
        self._state = None
        self._cond = threading.Condition()

        # Make this a daemon thread so that we can terminate immediately
        # even if it is testing a connection:
        threading.Thread(target=self._threadfunc, daemon=True).start()

    def _threadfunc(self) -> None:
        """Periodically update our connection status but idle if
         get_state has not been called recently."""

        _logger.info("Thread started.")

        while self._run:
            # Before we test the connection, see how long ago
            # the last call to get_state was. But in case a long
            # time has elapsed since get_state tried to awaken us and
            # our actually running again, do not go into the idle state
            # if this is a new call:
            with self._cond:
                if (self._last_call_time == self._start_time or
                time.monotonic() - self._last_call_time >= self._idle_timeout):
                    _logger.debug('Idling until the next call to get_state or shutdown at time %f', time.monotonic() - self._start_time)
                    self._idling = True
                    # Wait to be awakened by a get_status or shutodown call:
                    self._cond.wait()
                    if not self._run:
                        break
                    # We do not set self._idling to False until we have updated our connection state.
                    # This guarantees that we do not return any "stale" connection state to get_state.
                    # It also means that if we have multiple threads calling get_state, that we will
                    # not erroneously return a stale state to any of the threads.

            # We do no hold any locks while we test the connection:
            self._state = self._test_connection()

            with self._cond:
                if not self._run:
                    break
                self._idling = False
                self._cond.notify_all()
                # Wait to do next state update or shutodwn
                self._cond.wait(self._check_interval)

        _logger.info("Thread stopped.")

    def _test_connection(self) -> State:
        """Set current state and time of last check."""

        _logger.debug("Ping at time %f", time.monotonic() - self._start_time)

        try:
            with socket.socket() as s:
                s.settimeout(10)
                #s.connect(("8.8.8.8", 53))
                s.connect(("127.0.0.1", 80))
        except Exception as x:
            return State.NOT_CONNECTED
        else:
            return State.CONNECTED

    def get_state(self) -> State:
        """Return the current state."""

        with self._cond:
            if not self._run:
                return self._state
            if self._idling:
                self._cond.notify_all()
                while self._run and self._idling:
                    self._cond.wait()

        self._last_call_time = time.monotonic()  # Update

        _logger.debug('Returning state %s at time %f', self._state, time.monotonic() - self._start_time)
        return self._state

    def shutdown(self) -> None:
        with self._cond:
            if self._run:
                _logger.info("Shutting down ...")
                self._state = State.SHUTDOWN
                self._run = False
                self._cond.notify_all()

if __name__ == "__main__":

    c = ConnectivityChecker(2, 5)

    print("sleeping 10 seconds")
    time.sleep(10)

    print("checking five times in a row")
    print(c.get_state())
    print(c.get_state())
    print(c.get_state())
    print(c.get_state())
    print(c.get_state())

    print("sleeping 10 seconds")
    time.sleep(10)

    print("checking five times with 3 seconds between")
    for k in range(5):
        print(c.get_state())
        time.sleep(3)

    print("sleeping 10 seconds")
    time.sleep(10)

    c.shutdown()
    print(c.get_state())

Prints:

INFO:root:Thread started.
sleeping 10 seconds
DEBUG:root:Idling until the next call to get_state or shutdown at time 0.000000
checking five times in a row
DEBUG:root:Ping at time 10.000000
DEBUG:root:Returning state State.CONNECTED at time 10.032000
State.CONNECTED
DEBUG:root:Returning state State.CONNECTED at time 10.032000
State.CONNECTED
DEBUG:root:Returning state State.CONNECTED at time 10.032000
State.CONNECTED
DEBUG:root:Returning state State.CONNECTED at time 10.032000
State.CONNECTED
DEBUG:root:Returning state State.CONNECTED at time 10.032000
State.CONNECTED
sleeping 10 seconds
DEBUG:root:Ping at time 12.032000
DEBUG:root:Ping at time 14.047000
DEBUG:root:Idling until the next call to get_state or shutdown at time 16.078000
checking five times with 3 seconds between
DEBUG:root:Ping at time 20.032000
DEBUG:root:Returning state State.CONNECTED at time 20.032000
State.CONNECTED
DEBUG:root:Ping at time 22.047000
DEBUG:root:Returning state State.CONNECTED at time 23.032000
State.CONNECTED
DEBUG:root:Ping at time 24.063000
DEBUG:root:Returning state State.CONNECTED at time 26.032000
State.CONNECTED
DEBUG:root:Ping at time 26.078000
DEBUG:root:Ping at time 28.094000
DEBUG:root:Returning state State.CONNECTED at time 29.032000
State.CONNECTED
DEBUG:root:Ping at time 30.110000
DEBUG:root:Returning state State.CONNECTED at time 32.047000
State.CONNECTED
DEBUG:root:Ping at time 32.110000
DEBUG:root:Ping at time 34.141000
sleeping 10 seconds
DEBUG:root:Ping at time 36.157000
DEBUG:root:Idling until the next call to get_state or shutdown at time 38.157000
INFO:root:Shutting down ...
State.SHUTDOWN
INFO:root:Thread stopped.
\$\endgroup\$
4
  • \$\begingroup\$ Thanks for this! Some good tips in here. The only issue I have with your proposed implementation is that _test_connection is called while the cond var's mutex is locked, so if internet connectivity is poor and _test_connection takes a few seconds, get_state might block to wait on it when it acquires the lock at with self._cond, which doesn't meet one of the design goals. In the original implementation, _is_connected is called outside of any locks, and get_state only blocks the first time it's called in idle mode. Still, good stuff to think about, thanks again. \$\endgroup\$
    – Jason C
    Commented Jun 24 at 12:48
  • \$\begingroup\$ Btw to answer your question, in the original implementation if _get_state is called after shutdown, it just returns the last known state, which may be None (_get_state actually already has three possible return values, True, False, or, in the case of a shutdown in idle mode, None). \$\endgroup\$
    – Jason C
    Commented Jun 24 at 12:51
  • \$\begingroup\$ I have updated the code so that get_status will only wait a maximum amount of time for a status change before it decides to just use the current state. This amount of time I have set to self.__check_interval, but you can use any other value that you deem appropriate. \$\endgroup\$
    – Booboo
    Commented Jun 24 at 13:42
  • \$\begingroup\$ BTW, your implementation does not really handle long delays in _is_connected correctly. Place a call to time.sleep(4) right before you have the return true statement so that it takes at least 4 seconds to return a status. Then in the portion of your code where you sleep for 3 seconds before subsequent get_status calls, if you were to print out the current time before the return self._state statement , you will see that each print statement occurs 7 (3 + 4) seconds after the prior one. I will update my answer with something perhaps more to your liking. \$\endgroup\$
    – Booboo
    Commented Jun 24 at 15:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.