Skip to content

make sure to read the whole buffer in _sock_exact_recv() #133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jan 12, 2023

Conversation

vladak
Copy link
Contributor

@vladak vladak commented Dec 5, 2022

This change addresses the problem of returning incomplete buffer from _sock_exact_recv() in CPython.

Tested with:

#!/usr/bin/env python3

import adafruit_minimqtt.adafruit_minimqtt as MQTT
import socket
import ssl
import logging


def connect_hook(client, user_data, result, code):
    print(f"Connect: {user_data} {result} {code}")


def message_hook(client, topic, message):
    print(f"Message: topic='{topic}' message='{message}'")


broker = "test.mosquitto.org"
port = 1883

mqtt_client = MQTT.MQTT(
    broker=broker,
    port=port,
    socket_pool=socket,
    ssl_context=ssl.create_default_context(),
)


logging.basicConfig()
# mqtt_client.enable_logger(logging, log_level=logging.DEBUG)

# mqtt_client.on_connect = connect_hook
mqtt_client.on_message = message_hook

mqtt_client.connect()
mqtt_client.subscribe('#')

while True:
    try:
        mqtt_client.loop(5)
    except UnicodeDecodeError:
        pass

It survived the wild message traffic there for bunch of minutes.

@vladak
Copy link
Contributor Author

vladak commented Dec 6, 2022

I also used the above test code on QtPy, with the above code adapted for CircuitPython and WiFi, using the changed adafruit_minimqtt module and it ran fine for bunch of minutes. Eventually it got OSError (ETIMEDOUT) however that is probably not related to the changes. I made sure with inserted debug print the 'backward compat' branch in _sock_exact_recv() was taken. At first I thought that branch is used only by CPython, however seeing it is also used on microcontrollers, it seems this change will fix bunch of pre-existing issues. I am amazed how fast the program runs on the QtPy.

@tekktrik tekktrik requested a review from a team December 7, 2022 01:06
@dhalbert
Copy link
Contributor

dhalbert commented Dec 7, 2022

Eventually it got OSError (ETIMEDOUT) however that is probably not related to the changes.

What do you think this is related to? Do you think the server is doing rate limiting or something?

@vladak
Copy link
Contributor Author

vladak commented Dec 7, 2022

Eventually it got OSError (ETIMEDOUT) however that is probably not related to the changes.

What do you think this is related to? Do you think the server is doing rate limiting or something?

When I run this test with CPython on my laptop connected to WiFi hotspot on my phone that is using 4G to connect to the Internet, it just continuously runs until interrupted. Given that when the (slightly adapted) test code runs on the QtPy, using WiFi AP that is connected to super stable Ethernet based Internet upstream and considering it uses the same branch in _sock_exact_recv(), I'd attribute this to WiFi flakiness / network delays and/or to the lower layers (CircuitPython / the OS running on the chip). The way I look at this change is that this fixes a problem that allows to uncover more problems underneath.

pylint will like the code more
@dhalbert dhalbert requested a review from brentru January 11, 2023 01:20
@dhalbert
Copy link
Contributor

@brentru Could you take a look at this?

@brentru
Copy link
Member

brentru commented Jan 11, 2023

@dhalbert is there urgency? I am out today, returning tomorrow.

@dhalbert
Copy link
Contributor

@dhalbert is there urgency? I am out today, returning tomorrow.

NP, this was just hanging for a while.

@vladak vladak changed the title make sure to read the whole buffer in _sock_exact_recv() (CPython) make sure to read the whole buffer in _sock_exact_recv() Jan 12, 2023
Copy link
Member

@brentru brentru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the change I requested! LGTM

@brentru brentru merged commit 645ae31 into adafruit:main Jan 12, 2023
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jan 14, 2023
Updating https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT to 7.1.2 from 7.1.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#144 from vladak/f_strings
  > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#143 from vladak/network_manager_be_gone
  > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#133 from vladak/exact_recv_is_not

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Updated download stats for the libraries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants