Add unit test for fix of plugin EP library handle leak in PR 28396 by adrianlizarraga · Pull Request #28430 · microsoft/onnxruntime

adrianlizarraga · 2026-05-09T01:05:54Z

Description

Add a unit test that verifies RegisterExecutionProviderLibrary / UnregisterExecutionProviderLibrary does not leak the library handle (regression test for #28396).

ProviderLibrary::Load() loads the EP library and probes for the GetProvider symbol. Most plugin EP libraries don't export it, so the probe fails. Before #28396, Load() returned the error without calling Unload(), leaking a refcount.

Test approach

The test copies the EP library to a temporary directory with a unique filename, ensuring it has never been loaded in the process. After register + unregister, it checks that the library is fully unloaded (refcount == 0).

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>

Copilot

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>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

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>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

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>

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.

- 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>

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

…dence Copy the EP library to a temporary directory with a unique filename before testing register/unregister. This guarantees the library starts with refcount 0 regardless of test execution order, eliminating the GTEST_SKIP that could mask the very regression this test is designed to catch. - Use existing TemporaryDirectory (RAII) for cross-platform cleanup - Use std::filesystem::copy_file to create uniquely-named library copy - Remove the loaded-before GTEST_SKIP since the copy is always fresh - Keep GTEST_SKIP only for platforms without RTLD_NOLOAD support Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

edgchen1 · 2026-05-13T21:24:28Z

+  ASSERT_TRUE(IsLibraryLoaded(temp_library_path).value_or(false)) << "Library should be loaded after registration.";
+
+  // Unregister releases the EpLibraryPlugin's reference.
+  ort_env->UnregisterExecutionProviderLibrary(registration_name.c_str());


as this is updating ort_env, should we ensure that it gets called when leaving the scope? e.g., with gsl::finally

edgchen1 · 2026-05-13T21:25:55Z

+  return false;
+#else
+  // RTLD_NOLOAD is not available on this platform; cannot probe without loading.
+  (void)library_path;


Suggested change

(void)library_path;

static_cast<void>(library_path);

adrianlizarraga and others added 2 commits May 8, 2026 17:13

Add unit test for fix in PR #28396

9b3a242

adrianlizarraga requested a review from Copilot May 9, 2026 01:05

Copilot started reviewing on behalf of adrianlizarraga May 9, 2026 01:09 View session

Copilot AI reviewed May 9, 2026

View reviewed changes

Comment thread onnxruntime/test/autoep/test_registration.cc Outdated

Comment thread onnxruntime/test/autoep/test_registration.cc Outdated

adrianlizarraga and others added 5 commits May 11, 2026 10:38

Clean up

1a1d423

Merge branch 'main' into adrianl/EpLoadRefCountLeak_UnitTest

6bad160

adrianlizarraga requested a review from Copilot May 11, 2026 18:39

Copilot started reviewing on behalf of adrianlizarraga May 11, 2026 18:40 View session

Copilot AI reviewed May 11, 2026

View reviewed changes

Comment thread cmake/onnxruntime_unittests.cmake Outdated

adrianlizarraga requested a review from Copilot May 11, 2026 18:51

Copilot started reviewing on behalf of adrianlizarraga May 11, 2026 18:52 View session

Copilot AI reviewed May 11, 2026

View reviewed changes

adrianlizarraga requested a review from Copilot May 11, 2026 21:07

Copilot AI reviewed May 11, 2026

View reviewed changes

Copilot started reviewing on behalf of adrianlizarraga May 11, 2026 21:51 View session

adrianlizarraga and others added 2 commits May 11, 2026 16:20

Address more comments

98f2469

adrianlizarraga requested a review from Copilot May 12, 2026 00:01

Copilot started reviewing on behalf of adrianlizarraga May 12, 2026 00:02 View session

Copilot AI reviewed May 12, 2026

View reviewed changes

Comment thread onnxruntime/test/autoep/test_handle_leak.cc Outdated

adrianlizarraga and others added 3 commits May 13, 2026 11:12

Merge branch 'main' into adrianl/EpLoadRefCountLeak_UnitTest

0044615

Clean up

fd63fc9

adrianlizarraga requested a review from Copilot May 13, 2026 19:00

Copilot started reviewing on behalf of adrianlizarraga May 13, 2026 19:05 View session

Copilot AI reviewed May 13, 2026

View reviewed changes

adrianlizarraga changed the title ~~Add unit test for PR 28396~~ Add unit test for fix of plugin EP library handle leak in PR 28396 May 13, 2026

adrianlizarraga marked this pull request as ready for review May 13, 2026 20:15

adrianlizarraga mentioned this pull request May 13, 2026

Fix loader-refcount leak in ProviderLibrary::Load on GetProvider miss #28396

Merged

adrianlizarraga requested review from adrastogi and edgchen1 May 13, 2026 20:17

edgchen1 reviewed May 13, 2026

View reviewed changes

Conversation

adrianlizarraga commented May 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Test approach

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

edgchen1 May 13, 2026

Choose a reason for hiding this comment

Uh oh!

edgchen1 May 13, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adrianlizarraga commented May 9, 2026 •

edited

Loading