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.

Sign up for GitHub

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

Jump to bottom

Add RAII file wrappers to avoid resource leak #614

Merged
merged 5 commits into from
Feb 4, 2025
Merged

Add RAII file wrappers to avoid resource leak #614

merged 5 commits into from
Feb 4, 2025

Conversation

kingcrimsontianyu
Copy link
Contributor

Closes #607

@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Feb 4, 2025

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@kingcrimsontianyu kingcrimsontianyu changed the base branch from branch-25.02 to branch-25.04 February 4, 2025 15:30
@kingcrimsontianyu kingcrimsontianyu self-assigned this Feb 4, 2025
@kingcrimsontianyu kingcrimsontianyu added improvement Improves an existing functionality non-breaking Introduces a non-breaking change c++ Affects the C++ API of KvikIO labels Feb 4, 2025
@kingcrimsontianyu kingcrimsontianyu removed their assignment Feb 4, 2025
@kingcrimsontianyu kingcrimsontianyu marked this pull request as ready for review February 4, 2025 15:48
@kingcrimsontianyu kingcrimsontianyu requested review from a team as code owners February 4, 2025 15:48
@kingcrimsontianyu kingcrimsontianyu mentioned this pull request Feb 4, 2025
Open
KyleFromNVIDIA
KyleFromNVIDIA approved these changes Feb 4, 2025
View reviewed changes
Copy link
Contributor

@KyleFromNVIDIA KyleFromNVIDIA left a comment

Choose a reason for hiding this comment

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

Approved trivial CMake changes

madsbk
madsbk approved these changes Feb 4, 2025
View reviewed changes
Copy link
Member

@madsbk madsbk left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

cpp/include/kvikio/file_handle.hpp Outdated Show resolved Hide resolved
kingcrimsontianyu
kingcrimsontianyu commented Feb 4, 2025
View reviewed changes
cpp/src/file_utils.cpp
void CUFileHandleWrapper::unregister_handle() noexcept
{
if (registered()) {
cuFileAPI::instance().HandleDeregister(_handle);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nit: Non-success error codes from the system call close() and cuFileHandleDeregister() are currently left unhandled in order to keep the destructor noexcept. Maybe in the future we could consider having some logging mechanism or having some global error states for these rare but not impossible cases? Or simply removing the noexcept promise from the destructor? @madsbk

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep the dtor noexcept, but maybe print to stderr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Added to error.hpp/cpp . It looks like cuFileDeregister() returns void, so this rare error logging is only done for ::close() atm.

@madsbk
Copy link
Member

madsbk commented Feb 4, 2025

@kingcrimsontianyu, please feel free to merge when ready.

@kingcrimsontianyu
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit 255ed48 into rapidsai:branch-25.04 Feb 4, 2025
59 checks passed
@kingcrimsontianyu kingcrimsontianyu mentioned this pull request Feb 10, 2025
Closed
@kingcrimsontianyu kingcrimsontianyu self-assigned this Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@madsbk madsbk madsbk approved these changes

@vuule vuule vuule approved these changes

@KyleFromNVIDIA KyleFromNVIDIA KyleFromNVIDIA approved these changes

Assignees

@kingcrimsontianyu kingcrimsontianyu

Labels
c++ Affects the C++ API of KvikIO improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

FileHandle file resource may leak if the constructor throws an exception
4 participants
@kingcrimsontianyu @madsbk @vuule @KyleFromNVIDIA