Skip to content

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

Closed
flavio-fernandes opened this issue Jan 12, 2021 · 0 comments · Fixed by #55

Comments

@flavio-fernandes
Copy link
Contributor

flavio-fernandes commented Jan 12, 2021

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.

while True:
op = self._wait_for_msg(0.5)
if op == 208:
ping_resp = self._sock.recv(2)
if ping_resp[0] != 0x00:
raise MMQTTException("PINGRESP not returned from broker.")
return

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

1106.57: DEBUG - KeepAlive period elapsed - requesting a PINGRESP from the server...
1106.57: DEBUG - Sending PINGREQ
Writing: b'\xc0\x00'
1106.63: DEBUG - Checking PINGRESP
ESPSocket: 27 bytes available
Reading 1 bytes from ESP socket with status 4
ESPSocket: 26 bytes available
Reading 1 bytes from ESP socket with status 4
ESPSocket: 25 bytes available
Reading 2 bytes from ESP socket with status 4
ESPSocket: 23 bytes available
Reading 21 bytes from ESP socket with status 4
ESPSocket: 2 bytes available
Reading 2 bytes from ESP socket with status 4
Traceback (most recent call last):
  File "code.py", line 297, in <module>
  File "/lib/adafruit_minimqtt/adafruit_minimqtt.py", line 704, in loop
  File "/lib/adafruit_minimqtt/adafruit_minimqtt.py", line 407, in ping
  File "/lib/adafruit_minimqtt/adafruit_minimqtt.py", line 734, in _wait_for_msg
UnicodeError: 
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/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
Labels
None yet
Projects
None yet
1 participant