Skip to content

Conversation

@roxannerubin
Copy link
Contributor

@roxannerubin roxannerubin commented Oct 21, 2025

When ::connect() fails, the socket file descriptor was not closed before throwing an exception, leading to a file descriptor leak. This fix ensures the socket is properly closed on connection failure.

We had code that was retrying opening a modbus connection on an interval. When there was no device available, it would fail and create a new file descriptor every time. I tested this change and confirmed the leak resolved.

When ::connect() fails, the socket file descriptor was not closed before
throwing an exception, leading to a file descriptor leak. This fix ensures
the socket is properly closed on connection failure.

The issue occurs in the Connection::with() factory method where a socket
is created with ::socket() but not cleaned up if the subsequent ::connect()
call fails.
@roxannerubin roxannerubin force-pushed the fix/socket-leak-on-connect-failure branch from 4f72fc1 to 397a169 Compare October 21, 2025 20:04
@roxannerubin
Copy link
Contributor Author

@Mazurel @MightyPiggie @alessandromrc Here is a bugfix for a file descriptor leak that is affecting my application. Please review at your earliest convenience and let me know if there is anything else I should do. Thanks!

@alessandromrc
Copy link
Contributor

LGTM

@alessandromrc
Copy link
Contributor

When ::connect() fails, the socket file descriptor was not closed before throwing an exception, leading to a file descriptor leak. This fix ensures the socket is properly closed on connection failure.

We had code that was retrying opening a modbus connection on an interval. When there was no device available, it would fail and create a new file descriptor every time. I tested this change and confirmed the leak resolved.

Does make sense as it's throwing a runtime_error but not freeing the socket...

The ctor won't create the actual Connection instance, and so the dtor would never ever get called.

@roxannerubin
Copy link
Contributor Author

LGTM

Great - are you able to approve? @alessandromrc

@alessandromrc
Copy link
Contributor

LGTM

Great - are you able to approve? @alessandromrc

Sadly not, I only contributed to the repo, I have no power in merging or anything of that sort.

Also this repo seems pretty stale, so I doubt that it will ever get merged.

Copy link
Owner

@Mazurel Mazurel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice, thank you a lot for this, and sorry for late response.

@Mazurel Mazurel merged commit c94ac1a into Mazurel:master Oct 28, 2025
3 of 4 checks passed
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.

4 participants