Skip to content
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

Shutdown connection when socket gets disconnected. #29

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

frosch01
Copy link

Some words on my motivation for this PR:

While using the TLS transport I found the library to continuously print the error: "IoTask: Socket disconnected" in my logs. The only location of the print is read_packet(). I did some code review and this to be reason:

When in connected state, the return handler is handle_read() (via run_once_connected()). handle_read() sends the disconnected error into the RX channel but does not shutdown the connection in the IoTask. As sending the Error into the channel is successfully the handler also returns successfully and run_once_connected() is called again and again, leading always to the same disconnected error.

Unfortunately I was not able to find a way to reproduce this behavior in my setup. As the broker for me runs in some infrastructure I was not able to kill the broker for creating a situation leaving a dangling socket behind. I expect the behavior to be reproducible easily by just killing the broker and waiting until the socket gets closed by the kernel.

Even that I did not test, I think closing the connection as the socket reports itself as disconnect is a missing action in the I/O-Task. Independent whether the socket can be closed successfully or not by shutdown_conn(), at the end the connection state will be set to disconnected and resources should have been released. So the maximum pain to happen will be a second error printed. But 2 error prints is much less pain compared to having one error printed again and again until the user drops the connection.

In the mqttc example, there is actually a handler for the disconnected state. So this error will never be observed, however there should be possible more than a single disconnected error printed.

if the patch is wrong as the lib user is expected to handle the disconnected error, this perhaps should be mentioned explicitly in the documentation. In this case, I think there should be some hints on how this is intended to be handled by the user in the right way, too.

Sorry for not verifying this patch. If you are really unhappy with me, let me know and I will try to setup the test environment on my private HW. Perhaps you can give things a try on your environment which for sure will be ready to be used.

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.

2 participants