adafruit / Adafruit_CircuitPython_MiniMQTT

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

`free_socket` or `close_socket` ? #207

Closed o-control closed 4 months ago

o-control commented 4 months ago

This may be due to some quirk of my code, but I've found that with this line as written:

self._connection_manager.free_socket(self._sock)

the MQTT connection doesn't close on calling disconnect(). For me it does work if I change that line to:

self._connection_manager.close_socket(self._sock)

FoamyGuy commented 4 months ago

@justmobilize do you have any idea on this question? Are there situations where that needs to be close_socket() instead? or perhaps it should both close and free?

justmobilize commented 4 months ago

Since connection_manager now handles sockets, free_socket will mark the socket as free so it can be used again. This way if you ask for the same one again, it's faster since the SSL negotiation has already happened.

@o-control is this causing an error in your code?

o-control commented 4 months ago

Hi

If I use the free_socket code then after I've called connect and disconnect once, if I then call connect a second time I get the message "Repeated connect failures".

Looking at what's happening in connect: when it's called the second time, its first connection attempt gets "No data was sent, socket was closed" from the Wiznet_5k interface. The remaining connection attempts get "Socket already connected to mqtt://xxx.xxx.xxx.xxx:1883". At that point, if I call disconnect I get the message "MiniMQTT is not connected".

justmobilize commented 4 months ago

Sounds good. Let me take a look at it.

justmobilize commented 4 months ago

@brentru although I can update this to re-use the open socket, I would imagine without calling ping the socket would disconnect and so it's just better to fully close it out. Would you agree?

brentru commented 4 months ago

I would imagine without calling ping the socket would disconnect and so it's just better to fully close it out.

Per the MQTT spec, without a call to ping() within the KEEPALIVE time, the timer will expire and the MQTT broker will disconnect the client's session.

Would you agree?

I don't fully understand the use case on this issue

justmobilize commented 4 months ago

@brentru perfect.

The issue is that with the change to ConnectionManager, requests and minimqtt held onto sockets differently and I didn't catch it. I need to make sure that minimqtt fully closes it

o-control commented 4 months ago

Thanks everyone!

justmobilize commented 4 months ago

@o-control thanks for opening the issue