adafruit / Adafruit_CircuitPython_MiniMQTT

MQTT Client Library for CircuitPython
Other
80 stars 49 forks source link

Changed is_connected to return bool while maintaining error checking #125

Closed BiffoBear closed 1 year ago

BiffoBear commented 1 year ago

closes #109 Renamed original is_connected method to _connected to maintain internal connection status checking. Created a new is_connected function that returns a bool. This is a breaking change.

This change allows code to be written like:

while not mqtt_client.is_connected():
    try:
        mqtt_client.connect()
    except RuntimeError:
        print("Unable to connect. Waiting for 10 seconds before retrying…")
        time.sleep(10)
print("Connected to MQTT broker.")
BiffoBear commented 1 year ago

I think this is a good change, but I'm also not as familiar with MQTT applications. I think this is worth getting a second opinion since its also API breaking. I'll bring it up at the CircuitPython weekly meeting tomorrow!

After poking around in the code, I believe the is_connected() method is poorly named. The method does not reflect the state of the connection when it is called. It checks whether a network socket exists and whether an initial connection was successfully made to the MQTT broker when MQTT.connect() was called. The is_connected method acts as a gatekeeper so methods like publish() and ping() are not called before the connection to the broker has been made. This is why it returns True or raises an exception.

If the MQTT broker is down, the is_connected method will return True but any attempt to write to the broker will raise a BrokenPipeError: 32 exception.

After poking the code a bit more, MQTT.connect() returns a value if a connection with the broker was successfully initiated and raises exceptions for failed connections. Thus a while not mqtt_client.is_connected: loop is not required. In short, I no longer think that this breaking change is worth making.

Perhaps the method should be renamed _initial_connection_made() or something along those lines to make clear that it does not reflect the current connection status and is not particularly useful to the user.

BiffoBear commented 1 year ago

Thanks to @2bndy5 for clarifying my thinking and pointing out that the Paho implementation returns a bool. Changes requested made.