Close sockets in case of unsuccessful connection#72
Open
FlorianGerhardt wants to merge 2 commits intosmartrent:mainfrom
Open
Close sockets in case of unsuccessful connection#72FlorianGerhardt wants to merge 2 commits intosmartrent:mainfrom
FlorianGerhardt wants to merge 2 commits intosmartrent:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #52 by preventing socket/file-descriptor leaks during repeated MQTT reconnect attempts when a connection attempt fails after the socket has been opened (e.g., timeouts or refused CONNACK).
Changes:
- Refactors
do_connect/2to explicitly close the socket on unsuccessful post-connect outcomes (refused CONNACK, timeouts, and other errors). - Normalizes a post-connect timeout into
{:error, :connection_timeout}after closing the socket.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
687
to
+697
| %Connack{status: {:refused, _reason}} = connack -> | ||
| :ok = transport.close(socket) | ||
| connack | ||
|
|
||
| {:error, :timeout} -> | ||
| :ok = transport.close(socket) | ||
| {:error, :connection_timeout} | ||
|
|
||
| {:error, _reason} = error -> | ||
| :ok = transport.close(socket) | ||
| error |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Florian Gerhardt <florian.gerhardt92@gmail.com>
There was a problem hiding this comment.
Pull request overview
Fixes a socket/file-descriptor leak in Tortoise311.Connection by ensuring sockets are explicitly closed when a connection attempt fails after the socket has been created (e.g., timeouts while waiting for CONNACK).
Changes:
- Refactors
do_connect/2to keep the created socket in scope for subsequent failure handling. - Closes the socket on CONNACK refusal and on send/recv errors (including
:timeoutand:closed). - Normalizes certain failure returns to higher-level connection errors (e.g.,
{:error, :connection_timeout}).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+691
to
+701
| {:error, :timeout} -> | ||
| :ok = transport.close(socket) | ||
| {:error, :connection_timeout} | ||
|
|
||
| {:error, :closed} -> | ||
| :ok = transport.close(socket) | ||
| {:error, :server_closed_connection} | ||
|
|
||
| {:error, _reason} = error -> | ||
| :ok = transport.close(socket) | ||
| error |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #52
I had another device in the field with a firewall blocking the mqtt connection. Again, as in the issue, the device became unresponsive after some time. I noticed the same issue of file descriptors piling up and dug a bit deeper. It seems like the sockets are not getting closed if the connection times out. This fixes the issue.
The problem does not appear with no internet connection as the socket does not get created in that case.
Basically I checked with
File.ls!("/proc/APP_PID/fd") |> Enum.counthow many file descriptors are in use. The count increased everytime it tried connecting and timed out. The count went back to normal after stopping Tortoise311 Supervisor as the processes are linked.Looking forward to feedback!