Fix MethodBinding/OverloadMapper memory leak (#691)#2719
Merged
Conversation
MethodBinding and OverloadMapper held PyObject `target` references that were not disposed during tp_clear, leaving Python-side refcount drops to wait on the multi-hop .NET finalizer chain. They also shared the same C# PyObject instance across mp_subscript/Overloads paths, so freeing one could free the underlying Python object out from under the others. - ExtensionType: add virtual OnClear() hook called from tp_clear before the GCHandle is released, letting subclasses eagerly drop owned Python references. - MethodBinding/OverloadMapper: override OnClear to dispose `target`. (`targetType` is intentionally not disposed since Python types are long-lived and tracked by other caches.) - Take an independent INCREF'd PyObject copy at every site that hands a shared target into a new MethodBinding or OverloadMapper, so each wrapper owns its own reference. Result: the three _does_not_leak_memory tests drop from ~485 MB delta to ~10 KB delta on Python 3.14.
The previous 90% threshold (0.9 MB/iter against a 1 MB allocation) documented the issue but did not reproduce it: master leaks ~600-765 KB/iter, which the 0.9 MB threshold accepts as passing. Drop the threshold to 10% (104 KB/iter). On the 2026-05-09 verification run with Python 3.14 GIL on linux-aarch64: Without fix (master): ~572-765 KB/iter (FAIL) With fix (this branch): ~-500 B/iter (PASS) Margin is roughly 6x in either direction across .NET 8 and .NET 10, so the threshold cleanly separates buggy from fixed states without being sensitive to GC noise.
Member
|
If the objects are not shared anymore and are always owned by |
lostmsu
reviewed
May 11, 2026
Contributor
Author
|
Ownership is still explicit: |
- Handle the `PyType` reference in `OverloadMapper` and `MethodBinding` in the same way as the object reference - Unconditionally store the `PyType` of the object - Introduce `NewReference` helper function for the object and type passing - Fix the remaining missing reference count bump for the type (`MethodObject`) - As the count is now correct, `Dispose` the type as well
Member
|
@greateggsgreg I took the liberty of pushing my notes as a commit :)
If these are fine with you, I'd go ahead and merge this once the CI is through. |
Contributor
Author
|
Awesome, looks good. Thank you! |
This was referenced May 24, 2026
CurtHagenlocher
pushed a commit
to apache/arrow-dotnet
that referenced
this pull request
May 25, 2026
Updated [pythonnet](https://github.com/pythonnet/pythonnet) from 3.0.5 to 3.1.0. <details> <summary>Release notes</summary> _Sourced from [pythonnet's releases](https://github.com/pythonnet/pythonnet/releases)._ ## 3.1.0 ## What's Changed * ci: properly exclude job by @RobPasMue in pythonnet/pythonnet#2542 * `__delitem__` for `IList<T>` and `IDictionary<K,V>` by @lostmsu in pythonnet/pythonnet#2533 * Fix docs workflow by @filmor in pythonnet/pythonnet#2584 * Drop EOLd Python versions by @filmor in pythonnet/pythonnet#2632 * Bump setuptools and adjust license information by @filmor in pythonnet/pythonnet#2633 * Minimal .NET 8 usage changes by @filmor in pythonnet/pythonnet#2634 * Drop performance tests by @filmor in pythonnet/pythonnet#2636 * Properly detect availability of BinaryFormatter by @filmor in pythonnet/pythonnet#2639 * Use last compiler toolset version that support .NET 8 by @filmor in pythonnet/pythonnet#2640 * Add dependabot file by @filmor in pythonnet/pythonnet#2642 * Use official ARM runners by @filmor in pythonnet/pythonnet#2641 * Bump actions/upload-pages-artifact from 3 to 4 by @dependabot[bot] in pythonnet/pythonnet#2644 * Bump actions/setup-python from 2 to 6 by @dependabot[bot] in pythonnet/pythonnet#2646 * Bump actions/checkout from 2 to 5 by @dependabot[bot] in pythonnet/pythonnet#2648 * Bump actions/setup-dotnet from 1 to 5 by @dependabot[bot] in pythonnet/pythonnet#2645 * Use uv and derive as much as possible from the environment, if available by @filmor in pythonnet/pythonnet#2652 * Fixes for the uv CI by @filmor in pythonnet/pythonnet#2654 * Bump astral-sh/setup-uv from 6 to 7 by @dependabot[bot] in pythonnet/pythonnet#2656 * Bump actions/checkout from 5 to 6 by @dependabot[bot] in pythonnet/pythonnet#2663 * Ensure that the tests work even if BinaryFormatter is not available by @filmor in pythonnet/pythonnet#2638 * Bump NUnit3TestAdapter from 5.2.0 to 6.0.0 by @dependabot[bot] in pythonnet/pythonnet#2667 * Fix line endings by @filmor in pythonnet/pythonnet#2668 * Switch to .NET SDK 10 by @lostmsu in pythonnet/pythonnet#2684 * Python 3.14 by @filmor in pythonnet/pythonnet#2611 * CI Improvements by @filmor in pythonnet/pythonnet#2669 * Bump System.Reflection.Emit from 4.3.0 to 4.7.0 by @dependabot[bot] in pythonnet/pythonnet#2694 * Bump pytest from 9.0.2 to 9.0.3 in the uv group across 1 directory by @dependabot[bot] in pythonnet/pythonnet#2705 * CI Improvements by @filmor in pythonnet/pythonnet#2707 * Fix method memleak test by @filmor in pythonnet/pythonnet#2708 * Bump actions/upload-pages-artifact from 4 to 5 by @dependabot[bot] in pythonnet/pythonnet#2709 * Update furo requirement from >=2022.9.15 to >=2025.12.19 by @dependabot[bot] in pythonnet/pythonnet#2711 * Move documentation deps to pyproject.toml by @filmor in pythonnet/pythonnet#2714 * Support .NET Framework 4.6.1 by @Metadorius in pythonnet/pythonnet#2701 * Fix wheel tags by @filmor in pythonnet/pythonnet#2716 * Name missing from __all__ on re-import by @filmor in pythonnet/pythonnet#2717 * Add context manager protocol for .NET IDisposable types by @den-run-ai in pythonnet/pythonnet#2568 * Fix MethodBinding/OverloadMapper memory leak (#691) by @greateggsgreg in pythonnet/pythonnet#2719 * Bump urllib3 from 2.6.3 to 2.7.0 in the uv group across 1 directory by @dependabot[bot] in pythonnet/pythonnet#2723 * Update NUnit by @filmor in pythonnet/pythonnet#2724 * Silence compile-time warnings by @filmor in pythonnet/pythonnet#2725 * Implement support for DLR get/set by @filmor in pythonnet/pythonnet#2706 * Bump idna from 3.13 to 3.15 in the uv group across 1 directory by @dependabot[bot] in pythonnet/pythonnet#2726 ## New Contributors * @RobPasMue made their first contribution in pythonnet/pythonnet#2542 * @dependabot[bot] made their first contribution in pythonnet/pythonnet#2644 * @Metadorius made their first contribution in pythonnet/pythonnet#2701 **Full Changelog**: pythonnet/pythonnet@v3.0.5...v3.1.0 ## 3.1.0-rc1 ## What's Changed * CI Improvements by @filmor in pythonnet/pythonnet#2669 * Bump System.Reflection.Emit from 4.3.0 to 4.7.0 by @dependabot[bot] in pythonnet/pythonnet#2694 * Bump pytest from 9.0.2 to 9.0.3 in the uv group across 1 directory by @dependabot[bot] in pythonnet/pythonnet#2705 * CI Improvements by @filmor in pythonnet/pythonnet#2707 * Fix method memleak test by @filmor in pythonnet/pythonnet#2708 * Bump actions/upload-pages-artifact from 4 to 5 by @dependabot[bot] in pythonnet/pythonnet#2709 * Update furo requirement from >=2022.9.15 to >=2025.12.19 by @dependabot[bot] in pythonnet/pythonnet#2711 * Move documentation deps to pyproject.toml by @filmor in pythonnet/pythonnet#2714 * Support .NET Framework 4.6.1 by @Metadorius in pythonnet/pythonnet#2701 * Fix wheel tags by @filmor in pythonnet/pythonnet#2716 * Name missing from __all__ on re-import by @filmor in pythonnet/pythonnet#2717 * Add context manager protocol for .NET IDisposable types by @den-run-ai in pythonnet/pythonnet#2568 * Fix MethodBinding/OverloadMapper memory leak (#691) by @greateggsgreg in pythonnet/pythonnet#2719 * Bump urllib3 from 2.6.3 to 2.7.0 in the uv group across 1 directory by @dependabot[bot] in pythonnet/pythonnet#2723 * Update NUnit by @filmor in pythonnet/pythonnet#2724 * Silence compile-time warnings by @filmor in pythonnet/pythonnet#2725 * Implement support for DLR get/set by @filmor in pythonnet/pythonnet#2706 ## New Contributors * @Metadorius made their first contribution in pythonnet/pythonnet#2701 **Full Changelog**: pythonnet/pythonnet@v3.1.0-rc0...v3.1.0-rc1 ## 3.1.0-rc0 ## What's Changed * ci: properly exclude job by @RobPasMue in pythonnet/pythonnet#2542 * `__delitem__` for `IList<T>` and `IDictionary<K,V>` by @lostmsu in pythonnet/pythonnet#2533 * Fix docs workflow by @filmor in pythonnet/pythonnet#2584 * Drop EOLd Python versions by @filmor in pythonnet/pythonnet#2632 * Bump setuptools and adjust license information by @filmor in pythonnet/pythonnet#2633 * Minimal .NET 8 usage changes by @filmor in pythonnet/pythonnet#2634 * Drop performance tests by @filmor in pythonnet/pythonnet#2636 * Properly detect availability of BinaryFormatter by @filmor in pythonnet/pythonnet#2639 * Use last compiler toolset version that support .NET 8 by @filmor in pythonnet/pythonnet#2640 * Add dependabot file by @filmor in pythonnet/pythonnet#2642 * Use official ARM runners by @filmor in pythonnet/pythonnet#2641 * Use uv and derive as much as possible from the environment, if available by @filmor in pythonnet/pythonnet#2652 * Fixes for the uv CI by @filmor in pythonnet/pythonnet#2654 * Ensure that the tests work even if BinaryFormatter is not available by @filmor in pythonnet/pythonnet#2638 * Fix line endings by @filmor in pythonnet/pythonnet#2668 * Switch to .NET SDK 10 by @lostmsu in pythonnet/pythonnet#2684 * Python 3.14 by @filmor in pythonnet/pythonnet#2611 ## New Contributors * @RobPasMue made their first contribution in pythonnet/pythonnet#2542 * @dependabot[bot] made their first contribution in pythonnet/pythonnet#2644 **Full Changelog**: pythonnet/pythonnet@v3.0.5...v3.1.0-rc0 Commits viewable in [compare view](pythonnet/pythonnet@v3.0.5...v3.1.0). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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
3 participants
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.
Fixes #691.
Cause
MethodBindingandOverloadMapperhold aPyObject targetbut didn't release it ontp_clear, so the underlying CLR instance waited on the .NET finalizer chain to drop the refcount. They also shared the same C#PyObjectinstance acrossmp_subscript/Overloadspaths, so disposing one wrapper corrupted the others.Fix
ExtensionType: add virtualOnClear()hook called fromtp_clear.MethodBinding/OverloadMapper: overrideOnClearto disposetarget. (targetTypeleft alone — disposing it broke unrelated subclass tests.)new PyObject(self.target.Reference)so each wrapper owns its own INCREF'd reference.Tests
The three existing
*_does_not_leak_memorytests cover the three sharing sites but their 0.9 MB/iter threshold was too loose — master was leaking ~600 KB/iter and still passing. Tightened to 0.1 MB/iter (104 KB).Verification (Python 3.14 GIL, linux-aarch64)