Skip to content

free_socket or close_socket ? #207

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
o-control opened this issue Mar 4, 2024 · 9 comments · Fixed by #208
Closed

free_socket or close_socket ? #207

o-control opened this issue Mar 4, 2024 · 9 comments · Fixed by #208

Comments

@o-control
Copy link

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
Copy link
Contributor

FoamyGuy commented Mar 4, 2024

@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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

Sounds good. Let me take a look at it.

@justmobilize
Copy link
Collaborator

@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
Copy link
Member

brentru commented Mar 4, 2024

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
Copy link
Collaborator

@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
Copy link
Author

Thanks everyone!

@justmobilize
Copy link
Collaborator

@o-control thanks for opening the issue

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 a pull request may close this issue.

4 participants