-
Notifications
You must be signed in to change notification settings - Fork 51
Wrong semantics for size in socket.recv() causes sporatic failures in message parsing #54
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
Comments
flavio-fernandes
added a commit
to flavio-fernandes/Adafruit_CircuitPython_MiniMQTT
that referenced
this issue
Jan 13, 2021
This change addresses a few issues in the handling of the MQTT messages that caused the library to become unstable: - Add wapper for socket.recv() so that an exact number of bytes are read into the buffer before attempting to parse the MQTT message; - Fix handling of ping response packets as part of _wait_for_msg(), together with all other MQTT messages; - Fix disconnect so it can gracefully handle cases when socket writes are not possible. Also re-init _subscribed_topics as an empty list instead of None. Related-to adafruit/Adafruit_CircuitPython_ESP32SPI#102 Fixes adafruit/Adafruit_CircuitPython_PyPortal#98 Fixes adafruit#54 Signed-off-by: Flavio Fernandes <[email protected]>
This was referenced Jan 13, 2021
flavio-fernandes
added a commit
to flavio-fernandes/Adafruit_CircuitPython_MiniMQTT
that referenced
this issue
Jan 13, 2021
This change addresses a few issues in the handling of the MQTT messages that caused the library to become unstable: - Add wapper for socket.recv() so that an exact number of bytes are read into the buffer before attempting to parse the MQTT message; - Fix handling of ping response packets as part of _wait_for_msg(), together with all other MQTT messages; - Fix disconnect so it can gracefully handle cases when socket writes are not possible. Also re-init _subscribed_topics as an empty list instead of None. Related-to adafruit/Adafruit_CircuitPython_ESP32SPI#102 Fixes adafruit/Adafruit_CircuitPython_PyPortal#98 Fixes adafruit#54 Signed-off-by: Flavio Fernandes <[email protected]>
flavio-fernandes
added a commit
to flavio-fernandes/Adafruit_CircuitPython_MiniMQTT
that referenced
this issue
Jan 13, 2021
This change addresses a few issues in the handling of the MQTT messages that caused the library to become unstable: - Add wapper for socket.recv() so that an exact number of bytes are read into the buffer before attempting to parse the MQTT message; - Fix handling of ping response packets as part of _wait_for_msg(), together with all other MQTT messages; - Fix disconnect so it can gracefully handle cases when socket writes are not possible. Also re-init _subscribed_topics as an empty list instead of None. Related-to adafruit/Adafruit_CircuitPython_ESP32SPI#102 Fixes adafruit/Adafruit_CircuitPython_PyPortal#98 Fixes adafruit#54 Signed-off-by: Flavio Fernandes <[email protected]>
flavio-fernandes
added a commit
to flavio-fernandes/Adafruit_CircuitPython_MiniMQTT
that referenced
this issue
Jan 13, 2021
This change addresses a few issues in the handling of the MQTT messages that caused the library to become unstable: - Add wapper for socket.recv() so that an exact number of bytes are read into the buffer before attempting to parse the MQTT message; - Fix handling of ping response packets as part of _wait_for_msg(), together with all other MQTT messages; - Fix disconnect so it can gracefully handle cases when socket writes are not possible. Also re-init _subscribed_topics as an empty list instead of None. Related-to adafruit/Adafruit_CircuitPython_ESP32SPI#102 Fixes adafruit/Adafruit_CircuitPython_PyPortal#98 Fixes adafruit#54 Signed-off-by: Flavio Fernandes <[email protected]>
flavio-fernandes
added a commit
to flavio-fernandes/Adafruit_CircuitPython_MiniMQTT
that referenced
this issue
Jan 13, 2021
This change addresses a few issues in the handling of the MQTT messages that caused the library to become unstable: - Add wapper for socket.recv() so that an exact number of bytes are read into the buffer before attempting to parse the MQTT message; - Fix handling of ping response packets as part of _wait_for_msg(), together with all other MQTT messages; - Fix disconnect so it can gracefully handle cases when socket writes are not possible. Also re-init _subscribed_topics as an empty list instead of None. Related-to adafruit/Adafruit_CircuitPython_ESP32SPI#102 Fixes adafruit/Adafruit_CircuitPython_PyPortal#98 Fixes adafruit#54 Signed-off-by: Flavio Fernandes <[email protected]>
flavio-fernandes
added a commit
to flavio-fernandes/pyportal_station
that referenced
this issue
Jan 13, 2021
flavio-fernandes
added a commit
to flavio-fernandes/Adafruit_CircuitPython_MiniMQTT
that referenced
this issue
Jan 13, 2021
This change addresses a few issues in the handling of the MQTT messages that caused the library to become unstable: - Add wrapper for socket.recv() so that an exact number of bytes are read into the buffer before attempting to parse the MQTT message; - Fix handling of ping response packets as part of _wait_for_msg(), together with all other MQTT messages; - Fix disconnect so it can gracefully handle cases when socket writes are not possible. Also re-init _subscribed_topics as an empty list instead of None. Related-to adafruit/Adafruit_CircuitPython_ESP32SPI#102 Fixes adafruit/Adafruit_CircuitPython_PyPortal#98 Fixes adafruit#54 Signed-off-by: Flavio Fernandes <[email protected]>
flavio-fernandes
added a commit
to flavio-fernandes/Adafruit_CircuitPython_MiniMQTT
that referenced
this issue
Jan 14, 2021
This change addresses a few issues in the handling of the MQTT messages that caused the library to become unstable: - Add wrapper for socket.recv() so that an exact number of bytes are read into the buffer before attempting to parse the MQTT message; - Fix handling of ping response packets via _wait_for_msg(); - Fix disconnect so it can gracefully handle cases when socket writes are not possible. Also re-init _subscribed_topics as an empty list instead of None. Related-to adafruit/Adafruit_CircuitPython_ESP32SPI#102 Fixes adafruit/Adafruit_CircuitPython_PyPortal#98 Fixes adafruit#54 Signed-off-by: Flavio Fernandes <[email protected]>
flavio-fernandes
added a commit
to flavio-fernandes/pyportal_station
that referenced
this issue
Jan 14, 2021
flavio-fernandes
added a commit
to flavio-fernandes/pyportal_station
that referenced
this issue
Jan 16, 2021
rtwfroody
pushed a commit
to rtwfroody/Adafruit_CircuitPython_MiniMQTT
that referenced
this issue
Sep 18, 2022
This change addresses a few issues in the handling of the MQTT messages that caused the library to become unstable: - Add wrapper for socket.recv() so that an exact number of bytes are read into the buffer before attempting to parse the MQTT message; - Fix handling of ping response packets via _wait_for_msg(); - Fix disconnect so it can gracefully handle cases when socket writes are not possible. Also re-init _subscribed_topics as an empty list instead of None. Related-to adafruit/Adafruit_CircuitPython_ESP32SPI#102 Fixes adafruit/Adafruit_CircuitPython_PyPortal#98 Fixes adafruit#54 Signed-off-by: Flavio Fernandes <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The code for socket read has a buffer size param that indicates the maximum number of bytes
that can be returned within a single call:
https://github.com/adafruit/Adafruit_CircuitPython_ESP32SPI/blob/fce466bd2bb70ca86b79e5cb36bbaca00afacfd1/adafruit_esp32spi/adafruit_esp32spi_socket.py#L107-L112
Trouble is, MQTT is using that param as if that would be the minimum number of bytes that should be returned.
Also, ping method tries to drain buffer looking for the PINGRESP message and if that is not the message in the
receive buffer, it never reverts the 1 byte it took out of the buffer.
Adafruit_CircuitPython_MiniMQTT/adafruit_minimqtt/adafruit_minimqtt.py
Lines 406 to 412 in 682de96
After that point, MQTT broker and client are 'out of sync', causing any future exchanges to be 'out of whack'.
A better approach would be to flag that a ping response is expected, but handle it together with any other MQTT
message via _wait_for_msg
The text was updated successfully, but these errors were encountered: