-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Add unit test for fix of plugin EP library handle leak in PR 28396 #28430
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
Merged
+114
−0
Merged
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
9b3a242
Add unit test for fix in PR #28396
adrianlizarraga 5773f53
Add unit test for fix in PR #28396
adrianlizarraga 55ae8a5
Address PR review: use baseline comparison instead of drain loop
adrianlizarraga 6a88946
Use GTEST_SKIP when library is already loaded
adrianlizarraga c7c3c3f
Move handle leak test to separate binary for reliable detection
adrianlizarraga 1a1d423
Clean up
adrianlizarraga 6bad160
Merge branch 'main' into adrianl/EpLoadRefCountLeak_UnitTest
adrianlizarraga 712b1ff
Exclude test_handle_leak.cc from main autoep test binary
adrianlizarraga 9e529b2
Revert to GTEST_SKIP: remove separate binary to fix ARM64 Debug OOM
adrianlizarraga 2583a17
Address PR review: add missing headers and guard RTLD_NOLOAD
adrianlizarraga e8fbb5e
Return std::optional<bool> from IsLibraryLoaded for clarity
adrianlizarraga 98f2469
Address more comments
adrianlizarraga 0044615
Merge branch 'main' into adrianl/EpLoadRefCountLeak_UnitTest
adrianlizarraga 404a187
Use temp-copied library in handle leak test for process-state indepen…
adrianlizarraga fd63fc9
Clean up
adrianlizarraga 37fdf7a
Merge branch 'main' into adrianl/EpLoadRefCountLeak_UnitTest
adrianlizarraga 798f774
Clean up lib registration with RAII; Use static_cast
adrianlizarraga 98c8ab6
Use smaller scope
adrianlizarraga File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| #include <filesystem> | ||
| #include <gsl/gsl> | ||
| #include <memory> | ||
| #include <optional> | ||
| #include <string> | ||
| #include <gtest/gtest.h> | ||
|
|
||
| #include "core/session/onnxruntime_cxx_api.h" | ||
|
|
||
| #include "test/autoep/test_autoep_utils.h" | ||
| #include "test/util/include/file_util.h" | ||
| #include "test/util/include/temp_dir.h" | ||
|
|
||
| #if defined(_WIN32) | ||
| #include <windows.h> | ||
| #else | ||
| #include <dlfcn.h> | ||
| #endif | ||
|
|
||
| extern std::unique_ptr<Ort::Env> ort_env; | ||
|
adrianlizarraga marked this conversation as resolved.
|
||
|
|
||
| namespace onnxruntime { | ||
| namespace test { | ||
|
|
||
| namespace { | ||
|
|
||
| // Returns whether the library is currently mapped in the process, or std::nullopt if the platform | ||
| // does not support querying loaded-library state without side effects. | ||
| // On Windows, GetModuleHandleW queries by filename without incrementing the refcount. | ||
| // On Linux/macOS, dlopen with RTLD_NOLOAD probes without loading; if it succeeds it adds a | ||
| // refcount that we immediately release with dlclose. | ||
| std::optional<bool> IsLibraryLoaded(const std::filesystem::path& library_path) { | ||
| #if defined(_WIN32) | ||
| return GetModuleHandleW(library_path.filename().wstring().c_str()) != nullptr; | ||
| #else | ||
| #ifdef RTLD_NOLOAD | ||
| void* handle = dlopen(library_path.c_str(), RTLD_NOLOAD | RTLD_NOW); | ||
|
adrianlizarraga marked this conversation as resolved.
|
||
| if (handle) { | ||
| dlclose(handle); // Undo the refcount added by the RTLD_NOLOAD probe. | ||
| return true; | ||
| } | ||
| return false; | ||
| #else | ||
| // RTLD_NOLOAD is not available on this platform; cannot probe without loading. | ||
| static_cast<void>(library_path); | ||
| return std::nullopt; | ||
| #endif | ||
| #endif | ||
| } | ||
|
|
||
| } // namespace | ||
|
|
||
| // Verify that registering and unregistering a plugin EP library does not leak the library handle. | ||
| // | ||
| // ProviderLibrary::Load() loads the library then probes for the "GetProvider" symbol. Most plugin EP | ||
| // libraries do not export "GetProvider", so the probe fails. Without the fix (PR #28396), | ||
| // Load() returned the error without calling Unload(), leaving a leaked refcount. After | ||
| // UnregisterExecutionProviderLibrary released only the EpLibraryPlugin's reference, the library | ||
| // remained mapped in the process. | ||
| // | ||
| // To ensure this test is independent of process state (other tests may load the same EP library), | ||
| // we copy the library to a temporary directory with a unique filename. This guarantees the copy | ||
| // has never been loaded, so we can reliably detect refcount leaks via IsLibraryLoaded. | ||
| TEST(OrtEpLibrary, RegisterUnregisterDoesNotLeakLibraryHandle) { | ||
| const std::filesystem::path& original_library_path = Utils::example_ep_info.library_path; | ||
|
|
||
| // Use a unique registration name to avoid conflicts with other tests that may have | ||
| // registered the same EP library and failed to unregister it. | ||
| const std::string registration_name = "handle_leak_test_ep"; | ||
|
|
||
| // Copy the EP library to the temp directory with a unique filename so it is guaranteed to | ||
| // not already be loaded in this process. | ||
| TemporaryDirectory temp_dir(ORT_TSTR("test_handle_leak_temp")); | ||
| const std::filesystem::path temp_library_path = | ||
| std::filesystem::path(temp_dir.Path()) / | ||
| GetSharedLibraryFileName(ORT_TSTR("handle_leak_test_ep")); | ||
|
|
||
| std::error_code ec; | ||
| std::filesystem::copy_file(original_library_path, temp_library_path, | ||
| std::filesystem::copy_options::overwrite_existing, ec); | ||
| ASSERT_FALSE(ec) << "Failed to copy EP library to temp directory: " << ec.message(); | ||
|
|
||
| std::optional<bool> loaded_before = IsLibraryLoaded(temp_library_path); | ||
| if (!loaded_before.has_value()) { | ||
| GTEST_SKIP() << "Platform does not support querying loaded-library state."; | ||
| } | ||
|
|
||
| // The copy should not be loaded yet since we just created it with a unique name. | ||
| ASSERT_FALSE(*loaded_before) << "Freshly copied library should not already be loaded in the process."; | ||
|
|
||
| // Register the plugin EP library inside a smaller scope so that the gsl::finally cleanup | ||
| // calls UnregisterExecutionProviderLibrary exactly once when leaving the scope. | ||
| { | ||
| ASSERT_NO_THROW(ort_env->RegisterExecutionProviderLibrary(registration_name.c_str(), temp_library_path.c_str())); | ||
| auto cleanup_lib = gsl::finally([®istration_name] { | ||
| Ort::Status ignored{Ort::GetApi().UnregisterExecutionProviderLibrary(*ort_env, registration_name.c_str())}; | ||
|
adrianlizarraga marked this conversation as resolved.
|
||
| }); | ||
|
|
||
| // The library should be loaded now. | ||
| ASSERT_TRUE(IsLibraryLoaded(temp_library_path).value_or(false)) << "Library should be loaded after registration."; | ||
| } | ||
|
|
||
| // If the fix is applied, the library should be fully unloaded (refcount == 0). | ||
| // Without the fix, ProviderLibrary::Load() leaks a refcount so the library remains mapped. | ||
| EXPECT_FALSE(IsLibraryLoaded(temp_library_path).value_or(true)) | ||
| << "Library handle leaked: EP library is still loaded after UnregisterExecutionProviderLibrary. " | ||
| "This indicates ProviderLibrary::Load() did not call Unload() on GetProvider symbol miss."; | ||
| } | ||
|
|
||
| } // namespace test | ||
| } // namespace onnxruntime | ||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.