adafruit / Adafruit_CircuitPython_MiniMQTT

MQTT Client Library for CircuitPython
Other
72 stars 50 forks source link

variable header in connect() not clean on multiple connects #185

Closed vladak closed 7 months ago

vladak commented 7 months ago

I have a unit test for a MQTT server implementation (CPython) that goes like this:

import logging
import socket
import ssl
import time
import pytest

import adafruit_minimqtt.adafruit_minimqtt as MQTT

from ..common import mqtt_server

# Run multiple times to catch potentially erroneous reuse of client structures on the server.
@pytest.mark.parametrize("keep_alive_timeout", [8, 16])
def test_keepalive(keep_alive_timeout):
    """
    Connect with a particular keep alive timeout, sleep for double the keep alive timeout, send PINGREQ.
    """
    logging.basicConfig()
    logger = logging.getLogger(__name__)
    logger.setLevel(logging.DEBUG)

    host = "localhost"
    port = 1883
    mqtt_client = MQTT.MQTT(
        broker=host,
        port=port,
        socket_pool=socket,
        ssl_context=ssl.create_default_context(),
        connect_retries=1,
        recv_timeout=5,
        keep_alive=keep_alive_timeout,
    )

    logger.debug(f"connecting with keep alive = {keep_alive_timeout}")
    mqtt_client.connect()
    # The MQTT spec says not hearing from the client for 1.5 times keep alive timeout
    # means it can be considered dead and disconnected.
    sleep_period = 2 * keep_alive_timeout
    logger.debug(f"sleeping for {sleep_period} seconds")
    time.sleep(sleep_period)
    # Will wait for PINGRESP for keep_alive seconds. Should not get anything back.
    with pytest.raises(MQTT.MMQTTException):
        logger.debug("pinging the server")
        mqtt_client.ping()

This produces 2 MQTT TCP sessions. The CONNECT message in the first sessions is fine, however the keepalive value in the CONNECT message in the 2nd session is 24 (8+16 which is incidentally 1.5*16). This addition is caused by this code: https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT/blob/926846c45664c9155a67a11395387e5b53159be4/adafruit_minimqtt/adafruit_minimqtt.py#L588-L589 specifically the bitwise OR operation. This is because the 1st session will modify the MQTT_HDR_CONNECT array and the 2nd session will use the data. Yes, the whole trouble is because the assignment on https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT/blob/926846c45664c9155a67a11395387e5b53159be4/adafruit_minimqtt/adafruit_minimqtt.py#L573 is not a copy, but a reference.

If I add this line of code to the unit test before the MQTT() initialization then the keep alive value in the 2nd CONNECT message is sent correctly:

    MQTT.MQTT_HDR_CONNECT = bytearray(b"\x04MQTT\x04\x02\0\0")