Add unit test for PR 28396#28430
Draft
adrianlizarraga wants to merge 12 commits into
Draft
Conversation
Make the library handle leak test cross-platform (Windows and Linux) by extracting IsLibraryLoaded/ReleaseLibraryOnce helpers that use GetModuleHandleW/FreeLibrary on Windows and dlopen(RTLD_NOLOAD)/dlclose on Linux. Replace GTEST_SKIP with a drain loop that releases any leaked refcounts from prior tests, ensuring the test always runs regardless of execution order. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a regression unit test in the AutoEP test suite to ensure registering/unregistering a plugin EP library does not leak a dynamic library handle (specifically when probing for GetProvider fails).
Changes:
- Add platform-specific helpers to detect whether a shared library is loaded.
- Add a new gtest that registers/unregisters the example plugin EP library and asserts the library is fully unloaded afterward.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Remove ReleaseLibraryOnce and the unbounded drain loop. Instead, capture the library loaded state before register and assert it returns to that baseline after unregister. This avoids the double dlclose UB risk on Linux and the potential infinite loop. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the baseline comparison with GTEST_SKIP when the library is already loaded by a prior test. The boolean IsLibraryLoaded cannot detect refcount leaks when the library is already mapped (it cannot distinguish refcount N from N+1). The skip is honest about this limitation and still catches the regression when the test runs first. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move RegisterUnregisterDoesNotLeakLibraryHandle to a dedicated test binary (onnxruntime_autoep_handle_leak_test). This guarantees the plugin EP library starts with refcount 0, eliminating the need for GTEST_SKIP when other tests have already loaded the library. The new binary reuses the shared test_main.cc for main()/ort_env and links test_autoep_utils.cc for library path computation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The file(GLOB) picks up all *.cc in the autoep directory, so test_handle_leak.cc was being compiled into both the main onnxruntime_autoep_test binary and the isolated onnxruntime_autoep_handle_leak_test binary. The ASSERT_FALSE at test start would trip in the main binary since other tests load the same library first. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The separate test binary (onnxruntime_autoep_handle_leak_test) caused the ARM64 Linux Debug CI to run out of memory during linking. Revert to using GTEST_SKIP when the library is already loaded by another test in the same binary. Add a TODO comment documenting the separate-binary approach for when CI resources allow it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add explicit <memory> and <string> includes instead of relying on transitive includes. - Guard RTLD_NOLOAD with #ifdef for portability on platforms that may not define it; returns true (triggering GTEST_SKIP) when unavailable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Instead of returning a misleading 'true' when RTLD_NOLOAD is unavailable, return std::nullopt to explicitly indicate the platform cannot determine loaded-library state. The test skips with a clear message in that case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
+78
to
+82
| if (*loaded_before) { | ||
| GTEST_SKIP() << "Library is already loaded by another test in this binary. " | ||
| "Cannot detect refcount leaks with a boolean loaded/unloaded check. " | ||
| "See TODO above for the ideal separate-binary approach."; | ||
| } |
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.
Description
Motivation and Context