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

Fix fd leak in RingBuffer; add fd tracking to find leaks (default off; opt-in). #154

Merged
merged 2 commits into from
Jul 1, 2024

Conversation

tonyastolfi
Copy link
Collaborator

@tonyastolfi tonyastolfi commented Jun 28, 2024

This PR fixes a file descriptor (fd) leak in RingBuffer::Impl. It also adds some diagnostic tools to try to make it easier to find and diagnose future fd leaks.

  • System open should only be called from llfs::system_open3 and llfs::system_open2
  • All other syscalls that allocate and return file descriptors (e.g. eventfd and memfd) should be wrapped in llfs::maybe_track_fd
  • When fd tracking is enabled, any call to maybe_track_fd will add a stack trace associated with the passed fd to a global table
  • The llfs/track_fds.hpp header provides the following API for debugging fd leaks:
    • get_open_fds() returns a set of the fds currently open by the calling process
    • is_fd_tracking_enabled() and set_fd_tracking_enabled(bool) allow querying and modifying the tracker status
    • get_trace_for_fd(int fd) will return the last stack trace associated with the passed fd, if there is one
    • set_trace_for_fd(int fd, stacktrace) allows a stack trace to be manually associated with a given fd

I also took the opportunity to clean up the code in IoRing::File, eliminating redundancy by calling existing functions.

@tonyastolfi tonyastolfi self-assigned this Jun 28, 2024
Copy link
Collaborator

@gabrielbornstein gabrielbornstein left a comment

Choose a reason for hiding this comment

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

Overall the changes look very clean and reduce code in some very nice ways! A couple questions, a couple comments and suggestions.

@tonyastolfi tonyastolfi merged commit 883f5b6 into mathworks:main Jul 1, 2024
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.

2 participants