From 9de6f7bd071f60c67b42847f33453c0d5bd61c7a Mon Sep 17 00:00:00 2001 From: dse Date: Thu, 31 Aug 2017 19:34:31 +0400 Subject: [PATCH 01/16] Runtime/Shutdown loop stores old caches and static variables. It's produces bugs when CPython freeing up enough objects. --- CHANGELOG.md | 2 ++ src/runtime/assemblymanager.cs | 8 +++----- src/runtime/classderived.cs | 9 ++++++++- src/runtime/classmanager.cs | 5 +++++ src/runtime/genericutil.cs | 6 ++++++ src/runtime/moduleobject.cs | 11 +++++++++++ src/runtime/pyscope.cs | 7 ++++++- src/runtime/runtime.cs | 7 +++++++ src/runtime/typemanager.cs | 6 +++++- 9 files changed, 53 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d408a8d1..54b84841e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,8 @@ This document follows the conventions laid out in [Keep a CHANGELOG][]. ### Fixed +- Fixed secondary PythonEngine.Initialize call, all sensitive static variables now reseted. + This is a hidden bug. Once python cleaning up enough memory, objects from previous engine run becomes corrupted. - Fixed Visual Studio 2017 compat (#434) for setup.py - Fixed crash on exit of the Python interpreter if a python class derived from a .NET class has a `__namespace__` or `__assembly__` diff --git a/src/runtime/assemblymanager.cs b/src/runtime/assemblymanager.cs index 06a4449a2..34d08f0ec 100644 --- a/src/runtime/assemblymanager.cs +++ b/src/runtime/assemblymanager.cs @@ -17,13 +17,14 @@ internal class AssemblyManager { // modified from event handlers below, potentially triggered from different .NET threads // therefore this should be a ConcurrentDictionary - private static ConcurrentDictionary> namespaces; + private static ConcurrentDictionary> namespaces = + new ConcurrentDictionary>(); //private static Dictionary> generics; private static AssemblyLoadEventHandler lhandler; private static ResolveEventHandler rhandler; // updated only under GIL? - private static Dictionary probed; + private static Dictionary probed = new Dictionary(32); // modified from event handlers below, potentially triggered from different .NET threads private static AssemblyList assemblies; @@ -40,9 +41,6 @@ private AssemblyManager() /// internal static void Initialize() { - namespaces = new ConcurrentDictionary>(); - probed = new Dictionary(32); - //generics = new Dictionary>(); assemblies = new AssemblyList(16); pypath = new List(16); diff --git a/src/runtime/classderived.cs b/src/runtime/classderived.cs index 16d3b99db..ec3734ea5 100644 --- a/src/runtime/classderived.cs +++ b/src/runtime/classderived.cs @@ -1,8 +1,9 @@ -using System; +using System; using System.Collections.Generic; using System.Linq; using System.Reflection; using System.Reflection.Emit; +using System.Resources; using System.Runtime.InteropServices; using System.Threading.Tasks; @@ -32,6 +33,12 @@ static ClassDerivedObject() moduleBuilders = new Dictionary, ModuleBuilder>(); } + public static void Reset() + { + assemblyBuilders = new Dictionary(); + moduleBuilders = new Dictionary, ModuleBuilder>(); + } + internal ClassDerivedObject(Type tp) : base(tp) { } diff --git a/src/runtime/classmanager.cs b/src/runtime/classmanager.cs index 6a9d40ebd..0b084a49d 100644 --- a/src/runtime/classmanager.cs +++ b/src/runtime/classmanager.cs @@ -34,6 +34,11 @@ static ClassManager() dtype = typeof(MulticastDelegate); } + public static void Reset() + { + cache = new Dictionary(128); + } + /// /// Return the ClassBase-derived instance that implements a particular /// reflected managed type, creating it if it doesn't yet exist. diff --git a/src/runtime/genericutil.cs b/src/runtime/genericutil.cs index 9772d082f..3a230e12c 100644 --- a/src/runtime/genericutil.cs +++ b/src/runtime/genericutil.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Resources; namespace Python.Runtime { @@ -20,6 +21,11 @@ static GenericUtil() mapping = new Dictionary>>(); } + public static void Reset() + { + mapping = new Dictionary>>(); + } + /// /// Register a generic type that appears in a given namespace. /// diff --git a/src/runtime/moduleobject.cs b/src/runtime/moduleobject.cs index e683026f9..0ec9900dd 100644 --- a/src/runtime/moduleobject.cs +++ b/src/runtime/moduleobject.cs @@ -328,6 +328,17 @@ public CLRModule() : base("clr") } } + public static void Reset() + { + hacked = false; + interactive_preload = true; + preload = false; + + // XXX Test performance of new features // + _SuppressDocs = false; + _SuppressOverloads = false; + } + /// /// The initializing of the preload hook has to happen as late as /// possible since sys.ps1 is created after the CLR module is diff --git a/src/runtime/pyscope.cs b/src/runtime/pyscope.cs index 67f93c6e2..8e6957855 100644 --- a/src/runtime/pyscope.cs +++ b/src/runtime/pyscope.cs @@ -537,10 +537,15 @@ public void Dispose() public class PyScopeManager { - public readonly static PyScopeManager Global = new PyScopeManager(); + public static PyScopeManager Global; private Dictionary NamedScopes = new Dictionary(); + internal static void Reset() + { + Global = new PyScopeManager(); + } + internal PyScope NewScope(string name) { if (name == null) diff --git a/src/runtime/runtime.cs b/src/runtime/runtime.cs index abd0661a4..1625d36ea 100644 --- a/src/runtime/runtime.cs +++ b/src/runtime/runtime.cs @@ -221,6 +221,13 @@ internal static void Initialize() PyEval_InitThreads(); } + CLRModule.Reset(); + GenericUtil.Reset(); + PyScopeManager.Reset(); + ClassManager.Reset(); + ClassDerivedObject.Reset(); + TypeManager.Reset(); + IntPtr op; IntPtr dict; if (IsPython3) diff --git a/src/runtime/typemanager.cs b/src/runtime/typemanager.cs index 6570ee083..ececcc79c 100644 --- a/src/runtime/typemanager.cs +++ b/src/runtime/typemanager.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections; using System.Collections.Generic; using System.Reflection; @@ -21,6 +21,10 @@ static TypeManager() cache = new Dictionary(128); } + public static void Reset() + { + cache = new Dictionary(128); + } /// /// Given a managed Type derived from ExtensionType, get the handle to From e9e5c60641574504255695b9828aece6aff642ec Mon Sep 17 00:00:00 2001 From: dse Date: Sat, 2 Sep 2017 22:51:58 +0400 Subject: [PATCH 02/16] Added all tests finalizer routine. --- src/embed_tests/Python.EmbeddingTest.csproj | 1 + src/embed_tests/TestsSuite.cs | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+) create mode 100644 src/embed_tests/TestsSuite.cs diff --git a/src/embed_tests/Python.EmbeddingTest.csproj b/src/embed_tests/Python.EmbeddingTest.csproj index fe02b0526..dc5478575 100644 --- a/src/embed_tests/Python.EmbeddingTest.csproj +++ b/src/embed_tests/Python.EmbeddingTest.csproj @@ -102,6 +102,7 @@ + diff --git a/src/embed_tests/TestsSuite.cs b/src/embed_tests/TestsSuite.cs new file mode 100644 index 000000000..44ce2d4b8 --- /dev/null +++ b/src/embed_tests/TestsSuite.cs @@ -0,0 +1,18 @@ +using NUnit.Framework; +using Python.Runtime; + +namespace Python.EmbeddingTest +{ + [SetUpFixture] + public class TestsSuite + { + [OneTimeTearDown] + public void FinalCleanup() + { + if (PythonEngine.IsInitialized) + { + PythonEngine.Shutdown(); + } + } + } +} From 64acfcb34abac5c0fa3722eea44c758b96da9d49 Mon Sep 17 00:00:00 2001 From: dse Date: Sat, 2 Sep 2017 23:01:47 +0400 Subject: [PATCH 03/16] Bug: Py_Initialize/Py_Finalize calls during alive PythonEngine. Fixed. --- src/embed_tests/TestRuntime.cs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/embed_tests/TestRuntime.cs b/src/embed_tests/TestRuntime.cs index 2e0598da7..bd7e39a78 100644 --- a/src/embed_tests/TestRuntime.cs +++ b/src/embed_tests/TestRuntime.cs @@ -1,4 +1,4 @@ -using System; +using System; using NUnit.Framework; using Python.Runtime; @@ -6,10 +6,21 @@ namespace Python.EmbeddingTest { public class TestRuntime { + [OneTimeSetUp] + public void SetUp() + { + // We needs to ensure that no any engines are running. + if (PythonEngine.IsInitialized) + { + PythonEngine.Shutdown(); + } + } + [Test] public static void Py_IsInitializedValue() { - Runtime.Runtime.Py_Finalize(); // In case another test left it on. + // We defenitely knows that all engines are shut downed. + // Runtime.Runtime.Py_Finalize(); Assert.AreEqual(0, Runtime.Runtime.Py_IsInitialized()); Runtime.Runtime.Py_Initialize(); Assert.AreEqual(1, Runtime.Runtime.Py_IsInitialized()); From 7898833d2be9833021c20bbe606c3594fb728348 Mon Sep 17 00:00:00 2001 From: dse Date: Sat, 2 Sep 2017 22:48:05 +0400 Subject: [PATCH 04/16] Init/Shutdown state variable (IsFinalizing) fix. --- src/runtime/runtime.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/runtime/runtime.cs b/src/runtime/runtime.cs index 1625d36ea..164b1bfaa 100644 --- a/src/runtime/runtime.cs +++ b/src/runtime/runtime.cs @@ -221,6 +221,8 @@ internal static void Initialize() PyEval_InitThreads(); } + IsFinalizing = false; + CLRModule.Reset(); GenericUtil.Reset(); PyScopeManager.Reset(); From ed29200beba1a6f4918a25332a07d8b6695959dd Mon Sep 17 00:00:00 2001 From: dse Date: Wed, 6 Sep 2017 00:45:44 +0400 Subject: [PATCH 05/16] TestPythonEngineProperties fixes. --- src/embed_tests/TestPythonEngineProperties.cs | 40 +++++++++++++++++-- src/runtime/pythonengine.cs | 16 +++++--- 2 files changed, 47 insertions(+), 9 deletions(-) diff --git a/src/embed_tests/TestPythonEngineProperties.cs b/src/embed_tests/TestPythonEngineProperties.cs index 01c6ae7e3..d95942577 100644 --- a/src/embed_tests/TestPythonEngineProperties.cs +++ b/src/embed_tests/TestPythonEngineProperties.cs @@ -1,4 +1,5 @@ using System; +using System.Diagnostics; using NUnit.Framework; using Python.Runtime; @@ -109,18 +110,41 @@ public static void GetPythonHomeDefault() [Test] public void SetPythonHome() { + // We needs to ensure that engine was started and shutdown at least once before setting dummy home. + // Otherwise engine will not run with dummy path with random problem. + if (!PythonEngine.IsInitialized) + { + PythonEngine.Initialize(); + } + + PythonEngine.Shutdown(); + + var pythonHomeBackup = PythonEngine.PythonHome; + var pythonHome = "/dummypath/"; PythonEngine.PythonHome = pythonHome; PythonEngine.Initialize(); - Assert.AreEqual(pythonHome, PythonEngine.PythonHome); PythonEngine.Shutdown(); + + // Restoring valid pythonhome. + PythonEngine.PythonHome = pythonHomeBackup; } [Test] public void SetPythonHomeTwice() { + // We needs to ensure that engine was started and shutdown at least once before setting dummy home. + // Otherwise engine will not run with dummy path with random problem. + if (!PythonEngine.IsInitialized) + { + PythonEngine.Initialize(); + } + PythonEngine.Shutdown(); + + var pythonHomeBackup = PythonEngine.PythonHome; + var pythonHome = "/dummypath/"; PythonEngine.PythonHome = "/dummypath2/"; @@ -129,11 +153,20 @@ public void SetPythonHomeTwice() Assert.AreEqual(pythonHome, PythonEngine.PythonHome); PythonEngine.Shutdown(); + + PythonEngine.PythonHome = pythonHomeBackup; } [Test] public void SetProgramName() { + if (PythonEngine.IsInitialized) + { + PythonEngine.Shutdown(); + } + + var programNameBackup = PythonEngine.ProgramName; + var programName = "FooBar"; PythonEngine.ProgramName = programName; @@ -141,6 +174,8 @@ public void SetProgramName() Assert.AreEqual(programName, PythonEngine.ProgramName); PythonEngine.Shutdown(); + + PythonEngine.ProgramName = programNameBackup; } [Test] @@ -156,7 +191,7 @@ public void SetPythonPath() string path = PythonEngine.PythonPath; PythonEngine.Shutdown(); - PythonEngine.ProgramName = path; + PythonEngine.PythonPath = path; PythonEngine.Initialize(); Assert.AreEqual(path, PythonEngine.PythonPath); @@ -171,7 +206,6 @@ public void SetPythonPathExceptionOn27() Assert.Pass(); } - // Get previous path to avoid crashing Python PythonEngine.Initialize(); string path = PythonEngine.PythonPath; PythonEngine.Shutdown(); diff --git a/src/runtime/pythonengine.cs b/src/runtime/pythonengine.cs index a23c7ac79..76e954ac5 100644 --- a/src/runtime/pythonengine.cs +++ b/src/runtime/pythonengine.cs @@ -295,12 +295,16 @@ public static void Shutdown() if (initialized) { PyScopeManager.Global.Clear(); - Marshal.FreeHGlobal(_pythonHome); - _pythonHome = IntPtr.Zero; - Marshal.FreeHGlobal(_programName); - _programName = IntPtr.Zero; - Marshal.FreeHGlobal(_pythonPath); - _pythonPath = IntPtr.Zero; + // We should not release memory for variables that can be used without initialized python engine. + // It's assumed that Py_GetPythonHome returns valid string without engine initialize. Py_GetPythonHome will always return the + // same pointer that was passed before to Py_SetPythonHome and stored in the _pythonHome. + + ////Marshal.FreeHGlobal(_pythonHome); + ////_pythonHome = IntPtr.Zero; + ////Marshal.FreeHGlobal(_programName); + ////_programName = IntPtr.Zero; + ////Marshal.FreeHGlobal(_pythonPath); + ////_pythonPath = IntPtr.Zero; Runtime.Shutdown(); initialized = false; From 507edc9610f472ea72fb666979742d156e762887 Mon Sep 17 00:00:00 2001 From: dse Date: Tue, 10 Oct 2017 14:45:45 +0200 Subject: [PATCH 06/16] Update TestRuntime.cs --- src/embed_tests/TestRuntime.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/embed_tests/TestRuntime.cs b/src/embed_tests/TestRuntime.cs index bd7e39a78..9dfb04c9c 100644 --- a/src/embed_tests/TestRuntime.cs +++ b/src/embed_tests/TestRuntime.cs @@ -19,7 +19,7 @@ public void SetUp() [Test] public static void Py_IsInitializedValue() { - // We defenitely knows that all engines are shut downed. + // We know for sure that all engines are shut down. // Runtime.Runtime.Py_Finalize(); Assert.AreEqual(0, Runtime.Runtime.Py_IsInitialized()); Runtime.Runtime.Py_Initialize(); From e86405d8020aac622fc2026eaa614501edf10f90 Mon Sep 17 00:00:00 2001 From: dse Date: Tue, 5 Sep 2017 02:45:38 +0400 Subject: [PATCH 07/16] Upgrading NUnit to 3.8.1 for better mono stability and R# support. --- CHANGELOG.md | 2 +- NuGet.config | 2 +- src/embed_tests/Python.EmbeddingTest.15.csproj | 4 ++-- src/embed_tests/Python.EmbeddingTest.csproj | 4 ++-- src/embed_tests/packages.config | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 54b84841e..4a2a7e61d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ This document follows the conventions laid out in [Keep a CHANGELOG][]. - Added new build system (pythonnet.15.sln) based on dotnetcore-sdk/xplat(crossplatform msbuild). Currently there two side-by-side build systems that produces the same output (net40) from the same sources. After a some transition time, current (mono/ msbuild 14.0) build system will be removed. -- NUnit upgraded to 3.7 (eliminates travis-ci random bug) +- NUnit upgraded to 3.8.1 (eliminates travis-ci random bug) - Added `clr.GetClrType` (#432, #433) - Allowed passing `None` for nullable args (#460) - Added keyword arguments based on C# syntax for calling CPython methods (#461) diff --git a/NuGet.config b/NuGet.config index 719fbc83c..f270fb328 100644 --- a/NuGet.config +++ b/NuGet.config @@ -1,7 +1,7 @@  - + \ No newline at end of file diff --git a/src/embed_tests/Python.EmbeddingTest.15.csproj b/src/embed_tests/Python.EmbeddingTest.15.csproj index 92d55a7e0..89b4acdc4 100644 --- a/src/embed_tests/Python.EmbeddingTest.15.csproj +++ b/src/embed_tests/Python.EmbeddingTest.15.csproj @@ -76,10 +76,10 @@ - + - + diff --git a/src/embed_tests/Python.EmbeddingTest.csproj b/src/embed_tests/Python.EmbeddingTest.csproj index dc5478575..8387a53a5 100644 --- a/src/embed_tests/Python.EmbeddingTest.csproj +++ b/src/embed_tests/Python.EmbeddingTest.csproj @@ -70,8 +70,8 @@ - - ..\..\packages\NUnit.3.7.1\lib\net40\nunit.framework.dll + + ..\..\packages\NUnit.3.8.1\lib\net40\nunit.framework.dll diff --git a/src/embed_tests/packages.config b/src/embed_tests/packages.config index 8c175f441..dcf8f3448 100644 --- a/src/embed_tests/packages.config +++ b/src/embed_tests/packages.config @@ -1,5 +1,5 @@ - + From 36b23ef1f9af68015313b3bb482028d530ab2ef1 Mon Sep 17 00:00:00 2001 From: dse Date: Mon, 4 Sep 2017 11:51:48 +0400 Subject: [PATCH 08/16] Stability testing suite. --- .travis.yml | 2 +- CHANGELOG.md | 2 + ci/appveyor_run_tests.ps1 | 2 +- src/embed_tests/Program.cs | 45 ++++++++++++++++--- .../Python.EmbeddingTest.15.csproj | 2 +- src/embed_tests/Python.EmbeddingTest.csproj | 8 +++- src/embed_tests/TestExample.cs | 1 + src/embed_tests/packages.config | 1 + 8 files changed, 53 insertions(+), 10 deletions(-) diff --git a/.travis.yml b/.travis.yml index 3ab0e900f..3e62db243 100644 --- a/.travis.yml +++ b/.travis.yml @@ -97,7 +97,7 @@ install: script: - python -m pytest - - mono $NUNIT_PATH src/embed_tests/bin/Python.EmbeddingTest.dll + - mono $NUNIT_PATH src/embed_tests/bin/Python.EmbeddingTest.exe - if [[ $BUILD_OPTS == --xplat ]]; then dotnet src/embed_tests/bin/netcoreapp2.0_publish/Python.EmbeddingTest.dll; fi after_script: diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a2a7e61d..ac427ef2b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ This document follows the conventions laid out in [Keep a CHANGELOG][]. ## [unreleased][] ### Added +- Added tool for debugging floating bugs. Stable tests are executed in the loop. ~100 cycles is enough to pop up any bugs. + Usage: Python.EmbeddingTest.exe --loop --where="cat != Unstable" - Added support for embedding python into dotnet core 2.0 (NetStandard 2.0) - Added new build system (pythonnet.15.sln) based on dotnetcore-sdk/xplat(crossplatform msbuild). Currently there two side-by-side build systems that produces the same output (net40) from the same sources. diff --git a/ci/appveyor_run_tests.ps1 b/ci/appveyor_run_tests.ps1 index b45440fbe..2811c5bed 100644 --- a/ci/appveyor_run_tests.ps1 +++ b/ci/appveyor_run_tests.ps1 @@ -20,7 +20,7 @@ else{ $PY = Get-Command python # Can't use ".\build\*\Python.EmbeddingTest.dll". Missing framework files. -$CS_TESTS = ".\src\embed_tests\bin\Python.EmbeddingTest.dll" +$CS_TESTS = ".\src\embed_tests\bin\Python.EmbeddingTest.exe" $RUNTIME_DIR = ".\src\runtime\bin\" # Run python tests with C# coverage diff --git a/src/embed_tests/Program.cs b/src/embed_tests/Program.cs index b4439e3e4..03a752e89 100644 --- a/src/embed_tests/Program.cs +++ b/src/embed_tests/Program.cs @@ -1,8 +1,12 @@ using System; - +using System.Collections; +using System.Collections.Generic; +using System.Diagnostics; +using System.Linq; using NUnit.Common; using NUnitLite; +using Python.Runtime; namespace Python.EmbeddingTest { @@ -10,10 +14,41 @@ public class Program { public static int Main(string[] args) { - return new AutoRun(typeof(Program).Assembly).Execute( - args, - new ExtendedTextWrapper(Console.Out), - Console.In); + if (args.Contains("--loop")) + { + args = args.Where(x => x != "--loop").ToArray(); + int result; + int runNumber = 0; + string pathEnv = Environment.GetEnvironmentVariable("PATH"); + do + { + Console.ForegroundColor = ConsoleColor.Green; + Console.Error.WriteLine($"----- Run = {++runNumber}, MemUsage = {Process.GetCurrentProcess().WorkingSet64 / 1024 / 1024} Mb -----"); + Console.ForegroundColor = ConsoleColor.Gray; + + result = new AutoRun(typeof(Program).Assembly).Execute( + args, + new ExtendedTextWrapper(Console.Out), + Console.In); + + // Python does not see Environment.SetEnvironmentVariable changes. + // So we needs restore PATH variable in a pythonic way. + using (new PythonEngine()) + { + dynamic os = PythonEngine.ImportModule("os"); + os.environ["PATH"] = new PyString(pathEnv); + } + } while (true); + + return result; + } + else + { + return new AutoRun(typeof(Program).Assembly).Execute( + args, + new ExtendedTextWrapper(Console.Out), + Console.In); + } } } } diff --git a/src/embed_tests/Python.EmbeddingTest.15.csproj b/src/embed_tests/Python.EmbeddingTest.15.csproj index 89b4acdc4..24bead01f 100644 --- a/src/embed_tests/Python.EmbeddingTest.15.csproj +++ b/src/embed_tests/Python.EmbeddingTest.15.csproj @@ -5,7 +5,7 @@ net40;netcoreapp2.0 x64;x86 DebugMono;DebugMonoPY3;ReleaseMono;ReleaseMonoPY3;DebugWin;DebugWinPY3;ReleaseWin;ReleaseWinPY3 - Exe + Exe false Python.EmbeddingTest Python.EmbeddingTest diff --git a/src/embed_tests/Python.EmbeddingTest.csproj b/src/embed_tests/Python.EmbeddingTest.csproj index 8387a53a5..c63857920 100644 --- a/src/embed_tests/Python.EmbeddingTest.csproj +++ b/src/embed_tests/Python.EmbeddingTest.csproj @@ -4,7 +4,7 @@ Debug AnyCPU {4165C59D-2822-499F-A6DB-EACA4C331EB5} - Library + Exe Python.EmbeddingTest Python.EmbeddingTest bin\Python.EmbeddingTest.xml @@ -70,9 +70,12 @@ - + ..\..\packages\NUnit.3.8.1\lib\net40\nunit.framework.dll + + ..\..\packages\NUnitLite.3.8.1\lib\net40\nunitlite.dll + @@ -84,6 +87,7 @@ + diff --git a/src/embed_tests/TestExample.cs b/src/embed_tests/TestExample.cs index 0cf795f5d..047f397ab 100644 --- a/src/embed_tests/TestExample.cs +++ b/src/embed_tests/TestExample.cs @@ -5,6 +5,7 @@ namespace Python.EmbeddingTest { + [Category("Unstable")] public class TestExample { [OneTimeSetUp] diff --git a/src/embed_tests/packages.config b/src/embed_tests/packages.config index dcf8f3448..aa03916a2 100644 --- a/src/embed_tests/packages.config +++ b/src/embed_tests/packages.config @@ -1,5 +1,6 @@ + From 791e6278ee29a155207df7765151dd1901940df6 Mon Sep 17 00:00:00 2001 From: dse Date: Tue, 5 Sep 2017 02:49:33 +0400 Subject: [PATCH 09/16] Switching to NUnitLite test runner under linux. --- .travis.yml | 4 +--- CHANGELOG.md | 3 ++- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/.travis.yml b/.travis.yml index 3e62db243..472cfc240 100644 --- a/.travis.yml +++ b/.travis.yml @@ -8,7 +8,6 @@ matrix: - python: 2.7 env: &xplat-env - BUILD_OPTS=--xplat - - NUNIT_PATH=~/.nuget/packages/nunit.consolerunner/3.*/tools/nunit3-console.exe addons: &xplat-addons apt: sources: @@ -45,7 +44,6 @@ matrix: - python: 2.7 env: &classic-env - BUILD_OPTS= - - NUNIT_PATH=./packages/NUnit.*/tools/nunit3-console.exe - python: 3.3 env: *classic-env @@ -97,7 +95,7 @@ install: script: - python -m pytest - - mono $NUNIT_PATH src/embed_tests/bin/Python.EmbeddingTest.exe + - mono src/embed_tests/bin/Python.EmbeddingTest.exe - if [[ $BUILD_OPTS == --xplat ]]; then dotnet src/embed_tests/bin/netcoreapp2.0_publish/Python.EmbeddingTest.dll; fi after_script: diff --git a/CHANGELOG.md b/CHANGELOG.md index ac427ef2b..311b4e2d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,8 @@ This document follows the conventions laid out in [Keep a CHANGELOG][]. - Added new build system (pythonnet.15.sln) based on dotnetcore-sdk/xplat(crossplatform msbuild). Currently there two side-by-side build systems that produces the same output (net40) from the same sources. After a some transition time, current (mono/ msbuild 14.0) build system will be removed. -- NUnit upgraded to 3.8.1 (eliminates travis-ci random bug) +- Python.EmbeddingTest (net40) now tested through built-in NUnitLite in Travis-CI. (Solves NUnit vs Mono stability problems.) +- NUnit upgraded to 3.8.1, Python.EmbeddingTest now executable with the NUnitLite self-tester. - Added `clr.GetClrType` (#432, #433) - Allowed passing `None` for nullable args (#460) - Added keyword arguments based on C# syntax for calling CPython methods (#461) From a3f26c264a2c6d3d569fb85f537c54eb5ddde4f6 Mon Sep 17 00:00:00 2001 From: dse Date: Sat, 2 Sep 2017 03:21:56 +0400 Subject: [PATCH 10/16] Valid garbage collection implementation. --- CHANGELOG.md | 2 +- src/embed_tests/Python.EmbeddingTest.csproj | 1 + src/embed_tests/TestFinalizer.cs | 68 ++++++++ src/runtime/Python.Runtime.csproj | 1 + src/runtime/delegatemanager.cs | 16 +- src/runtime/pyobject.cs | 44 +++-- src/runtime/pyreferencedecrementer.cs | 171 ++++++++++++++++++++ src/runtime/pyscope.cs | 53 ++++-- src/runtime/pythonengine.cs | 22 ++- src/runtime/pythonexception.cs | 65 ++++++-- 10 files changed, 390 insertions(+), 53 deletions(-) create mode 100644 src/embed_tests/TestFinalizer.cs create mode 100644 src/runtime/pyreferencedecrementer.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 311b4e2d9..a841ad5f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,7 +23,7 @@ This document follows the conventions laid out in [Keep a CHANGELOG][]. ### Changed ### Fixed - +- Fixed memory leaks, caused by non-working PyObject, PythonException, DelecateManager->Dispatcher finalizers. - Fixed secondary PythonEngine.Initialize call, all sensitive static variables now reseted. This is a hidden bug. Once python cleaning up enough memory, objects from previous engine run becomes corrupted. - Fixed Visual Studio 2017 compat (#434) for setup.py diff --git a/src/embed_tests/Python.EmbeddingTest.csproj b/src/embed_tests/Python.EmbeddingTest.csproj index c63857920..bbb35a8e0 100644 --- a/src/embed_tests/Python.EmbeddingTest.csproj +++ b/src/embed_tests/Python.EmbeddingTest.csproj @@ -106,6 +106,7 @@ + diff --git a/src/embed_tests/TestFinalizer.cs b/src/embed_tests/TestFinalizer.cs new file mode 100644 index 000000000..cfb8a491f --- /dev/null +++ b/src/embed_tests/TestFinalizer.cs @@ -0,0 +1,68 @@ +using System; +using System.IO; +using System.Threading; +using System.Threading.Tasks; +using NUnit.Framework; +using Python.Runtime; + +namespace Python.EmbeddingTest +{ + public class TestFinalizer + { + [OneTimeSetUp] + public void SetUp() + { + PythonEngine.Initialize(); + } + + [OneTimeTearDown] + public void Dispose() + { + PythonEngine.Shutdown(); + } + + [Test] + public void TestClrObjectFullRelease() + { + var gs = PythonEngine.BeginAllowThreads(); + + WeakReference weakRef; + try + { + var weakRefCreateTask = Task.Factory.StartNew(() => + { + using (Py.GIL()) + { + byte[] testObject = new byte[100]; + var testObjectWeakReference = new WeakReference(testObject); + + dynamic pyList = new PyList(); + pyList.append(testObject); + return testObjectWeakReference; + } + }); + + weakRef = weakRefCreateTask.Result; + } + finally + { + PythonEngine.EndAllowThreads(gs); + } + + // Triggering C# finalizer (PyList ref should be scheduled to decref). + GC.Collect(); + GC.WaitForPendingFinalizers(); + + // Forcing dec reference thread to wakeup and decref PyList. + PythonEngine.EndAllowThreads(PythonEngine.BeginAllowThreads()); + Thread.Sleep(200); + PythonEngine.CurrentRefDecrementer.WaitForPendingDecReferences(); + + // Now python free up GCHandle on CLRObject and subsequent GC should fully remove testObject. + GC.Collect(); + GC.WaitForPendingFinalizers(); + + Assert.IsFalse(weakRef.IsAlive, "Clr object should be collected."); + } + } +} diff --git a/src/runtime/Python.Runtime.csproj b/src/runtime/Python.Runtime.csproj index 82825a626..b36f228b2 100644 --- a/src/runtime/Python.Runtime.csproj +++ b/src/runtime/Python.Runtime.csproj @@ -129,6 +129,7 @@ + diff --git a/src/runtime/delegatemanager.cs b/src/runtime/delegatemanager.cs index 7632816d1..54256ac17 100644 --- a/src/runtime/delegatemanager.cs +++ b/src/runtime/delegatemanager.cs @@ -185,9 +185,12 @@ public class Dispatcher { public IntPtr target; public Type dtype; + private readonly PyReferenceDecrementer _referenceDecrementer; public Dispatcher(IntPtr target, Type dtype) { + _referenceDecrementer = PythonEngine.CurrentRefDecrementer; + Runtime.XIncref(target); this.target = target; this.dtype = dtype; @@ -195,18 +198,7 @@ public Dispatcher(IntPtr target, Type dtype) ~Dispatcher() { - // We needs to disable Finalizers until it's valid implementation. - // Current implementation can produce low probability floating bugs. - return; - - // Note: the managed GC thread can run and try to free one of - // these *after* the Python runtime has been finalized! - if (Runtime.Py_IsInitialized() > 0) - { - IntPtr gs = PythonEngine.AcquireLock(); - Runtime.XDecref(target); - PythonEngine.ReleaseLock(gs); - } + _referenceDecrementer?.ScheduleDecRef(target); } public object Dispatch(ArrayList args) diff --git a/src/runtime/pyobject.cs b/src/runtime/pyobject.cs index 5900e80b7..68878bf39 100644 --- a/src/runtime/pyobject.cs +++ b/src/runtime/pyobject.cs @@ -16,6 +16,7 @@ public class PyObject : DynamicObject, IDisposable { protected internal IntPtr obj = IntPtr.Zero; private bool disposed = false; + private readonly PyReferenceDecrementer _referenceDecrementer; /// /// PyObject Constructor @@ -26,7 +27,7 @@ public class PyObject : DynamicObject, IDisposable /// and the reference will be DECREFed when the PyObject is garbage /// collected or explicitly disposed. /// - public PyObject(IntPtr ptr) + public PyObject(IntPtr ptr): this() { obj = ptr; } @@ -36,6 +37,7 @@ public PyObject(IntPtr ptr) protected PyObject() { + _referenceDecrementer = PythonEngine.CurrentRefDecrementer; } // Ensure that encapsulated Python object is decref'ed appropriately @@ -43,11 +45,7 @@ protected PyObject() ~PyObject() { - // We needs to disable Finalizers until it's valid implementation. - // Current implementation can produce low probability floating bugs. - return; - - Dispose(); + Dispose(false); } @@ -138,21 +136,43 @@ protected virtual void Dispose(bool disposing) { if (!disposed) { - if (Runtime.Py_IsInitialized() > 0 && !Runtime.IsFinalizing) + disposed = true; + + if (disposing) { - IntPtr gs = PythonEngine.AcquireLock(); - Runtime.XDecref(obj); + try + { + if (Runtime.Py_IsInitialized() > 0 && !Runtime.IsFinalizing) + { + IntPtr gs = PythonEngine.AcquireLock(); + try + { + Runtime.XDecref(obj); + obj = IntPtr.Zero; + } + finally + { + PythonEngine.ReleaseLock(gs); + } + } + } + catch + { + // Do nothing. + } + } + else + { + _referenceDecrementer?.ScheduleDecRef(obj); obj = IntPtr.Zero; - PythonEngine.ReleaseLock(gs); } - disposed = true; } } public void Dispose() { - Dispose(true); GC.SuppressFinalize(this); + Dispose(true); } diff --git a/src/runtime/pyreferencedecrementer.cs b/src/runtime/pyreferencedecrementer.cs new file mode 100644 index 000000000..26bbf6a00 --- /dev/null +++ b/src/runtime/pyreferencedecrementer.cs @@ -0,0 +1,171 @@ +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Diagnostics; +using System.Threading; +using System.Threading.Tasks; + +namespace Python.Runtime +{ + internal class PyReferenceDecrementer : IDisposable + { + private static readonly DedicatedThreadTaskScheduler DedicatedThreadTaskScheduler = new DedicatedThreadTaskScheduler(); + + private readonly BlockingCollection _asyncDecRefQueue = new BlockingCollection(); + + private CancellationTokenSource _cts; + private CancellationToken _ct; + private Task _backgroundWorkerTask; + + public PyReferenceDecrementer() + { + InitDecRefThread(); + } + + public void ScheduleDecRef(IntPtr pyRef) + { + // ReSharper disable once MethodSupportsCancellation + _asyncDecRefQueue.Add(pyRef); + } + + internal void WaitForPendingDecReferences() + { + ShutdownDecRefThread(); + InitDecRefThread(); + } + + private void ShutdownDecRefThread() + { + _cts?.Cancel(); + try + { + IntPtr ts = IntPtr.Zero; + if (Runtime.PyGILState_GetThisThreadState() != IntPtr.Zero) + { + ts = Runtime.PyEval_SaveThread(); + } + try + { + // ReSharper disable once MethodSupportsCancellation + _backgroundWorkerTask.Wait(); + } + catch (AggregateException ex) + { + if (!(ex.InnerException is OperationCanceledException)) + { + throw; + } + } + finally + { + if (ts != IntPtr.Zero) + { + Runtime.PyEval_RestoreThread(ts); + } + } + } + catch + { + // Just stopping background thread. + } + + _cts = null; + _ct = default(CancellationToken); + + _backgroundWorkerTask = null; + } + + private void InitDecRefThread() + { + _cts = new CancellationTokenSource(); + _ct = _cts.Token; + + _backgroundWorkerTask = Task.Factory.StartNew(WorkerThread, _ct, TaskCreationOptions.LongRunning, + DedicatedThreadTaskScheduler); + } + + private void WorkerThread() + { + while (true) + { + IntPtr refToDecrease = _asyncDecRefQueue.Take(_ct); ; + + try + { + + IntPtr gs = PythonEngine.AcquireLock(); + try + { + do + { + Runtime.XDecref(refToDecrease); + } while (_asyncDecRefQueue.TryTake(out refToDecrease)); + } + finally + { + PythonEngine.ReleaseLock(gs); + } + } + catch + { + // Nothing to do in this case. + } + } + } + + public void Dispose() + { + ShutdownDecRefThread(); + } + } + + + /// + /// Scheduler that uses only one thread for all scheduled task. + /// + internal class DedicatedThreadTaskScheduler : TaskScheduler + { + private readonly BlockingCollection _queuedTasks = new BlockingCollection(); + + + /// + /// Initializes a new instance of the class. + /// + public DedicatedThreadTaskScheduler() + { + var thread = new Thread(WorkerThreadProc); + thread.IsBackground = true; + thread.Start(); + } + + /// + protected override IEnumerable GetScheduledTasks() + { + return _queuedTasks; + } + + /// + protected override void QueueTask(Task task) + { + _queuedTasks.Add(task); + } + + /// + protected override bool TryExecuteTaskInline(Task task, bool taskWasPreviouslyQueued) + { + return false; + } + + private void WorkerThreadProc() + { + for (;;) + { + Task dequeuedTask = _queuedTasks.Take(); + + // This is synchronous execution. + bool taskExecuted = TryExecuteTask(dequeuedTask); + Debug.Assert(taskExecuted, "DedicatedThread task have some problem."); + } + } + } +} diff --git a/src/runtime/pyscope.cs b/src/runtime/pyscope.cs index 8e6957855..2b38e460d 100644 --- a/src/runtime/pyscope.cs +++ b/src/runtime/pyscope.cs @@ -24,6 +24,8 @@ public class PyGILAttribute : Attribute [PyGIL] public class PyScope : DynamicObject, IDisposable { + private readonly PyReferenceDecrementer _referenceDecrementer; + public readonly string Name; /// @@ -57,6 +59,8 @@ public class PyScope : DynamicObject, IDisposable /// internal PyScope(IntPtr ptr, PyScopeManager manager) { + _referenceDecrementer = PythonEngine.CurrentRefDecrementer; + if (!Runtime.PyType_IsSubtype(Runtime.PyObject_TYPE(ptr), Runtime.PyModuleType)) { throw new PyScopeException("object is not a module"); @@ -514,24 +518,51 @@ private void Check() } } - public void Dispose() + protected virtual void Dispose(bool disposing) { - if (_isDisposed) + if (!_isDisposed) { - return; + _isDisposed = true; + + if (disposing) + { + try + { + if (Runtime.Py_IsInitialized() > 0 && !Runtime.IsFinalizing) + { + IntPtr gs = PythonEngine.AcquireLock(); + try + { + Runtime.XDecref(obj); + } + finally + { + PythonEngine.ReleaseLock(gs); + } + } + } + catch + { + // Do nothing. + } + OnDispose?.Invoke(this); + } + else + { + _referenceDecrementer?.ScheduleDecRef(obj); + } } - _isDisposed = true; - Runtime.XDecref(obj); - this.OnDispose?.Invoke(this); } - ~PyScope() + public void Dispose() { - // We needs to disable Finalizers until it's valid implementation. - // Current implementation can produce low probability floating bugs. - return; + GC.SuppressFinalize(this); + Dispose(true); + } - Dispose(); + ~PyScope() + { + Dispose(false); } } diff --git a/src/runtime/pythonengine.cs b/src/runtime/pythonengine.cs index 76e954ac5..7bbc842cf 100644 --- a/src/runtime/pythonengine.cs +++ b/src/runtime/pythonengine.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.IO; using System.Linq; @@ -130,6 +130,8 @@ public static string Compiler get { return Marshal.PtrToStringAnsi(Runtime.Py_GetCompiler()); } } + internal static PyReferenceDecrementer CurrentRefDecrementer { get; private set; } + public static int RunSimpleString(string code) { return Runtime.PyRun_SimpleString(code); @@ -215,6 +217,8 @@ public static void Initialize(IEnumerable args, bool setSysArgv = true) } key.Dispose(); } + + CurrentRefDecrementer = new PyReferenceDecrementer(); } finally { @@ -294,6 +298,9 @@ public static void Shutdown() { if (initialized) { + CurrentRefDecrementer?.Dispose(); + CurrentRefDecrementer = null; + PyScopeManager.Global.Clear(); // We should not release memory for variables that can be used without initialized python engine. // It's assumed that Py_GetPythonHome returns valid string without engine initialize. Py_GetPythonHome will always return the @@ -566,15 +573,24 @@ internal GILState() state = PythonEngine.AcquireLock(); } + protected virtual void Dispose(bool disposing) + { + //ReleeaseLock is thread bound and if it's called in finalizer thread it can wrongly release lock. + if (disposing) + { + PythonEngine.ReleaseLock(state); + } + } + public void Dispose() { - PythonEngine.ReleaseLock(state); GC.SuppressFinalize(this); + Dispose(true); } ~GILState() { - Dispose(); + Dispose(false); } } diff --git a/src/runtime/pythonexception.cs b/src/runtime/pythonexception.cs index 4031b0526..3ce017851 100644 --- a/src/runtime/pythonexception.cs +++ b/src/runtime/pythonexception.cs @@ -8,6 +8,7 @@ namespace Python.Runtime /// public class PythonException : System.Exception { + private readonly PyReferenceDecrementer _referenceDecrementer; private IntPtr _pyType = IntPtr.Zero; private IntPtr _pyValue = IntPtr.Zero; private IntPtr _pyTB = IntPtr.Zero; @@ -17,6 +18,7 @@ public class PythonException : System.Exception public PythonException() { + _referenceDecrementer = PythonEngine.CurrentRefDecrementer; IntPtr gs = PythonEngine.AcquireLock(); Runtime.PyErr_Fetch(ref _pyType, ref _pyValue, ref _pyTB); Runtime.XIncref(_pyType); @@ -57,11 +59,7 @@ public PythonException() ~PythonException() { - // We needs to disable Finalizers until it's valid implementation. - // Current implementation can produce low probability floating bugs. - return; - - Dispose(); + Dispose(false); } /// @@ -143,23 +141,62 @@ public override string StackTrace /// See GH#397 and GH#400. /// public void Dispose() + { + GC.SuppressFinalize(this); + Dispose(true); + } + + protected void Dispose(bool disposing) { if (!disposed) { - if (Runtime.Py_IsInitialized() > 0 && !Runtime.IsFinalizing) + disposed = true; + + if (disposing) { - IntPtr gs = PythonEngine.AcquireLock(); - Runtime.XDecref(_pyType); - Runtime.XDecref(_pyValue); - // XXX Do we ever get TraceBack? // + if (Runtime.Py_IsInitialized() > 0 && !Runtime.IsFinalizing) + { + try + { + IntPtr gs = PythonEngine.AcquireLock(); + try + { + Runtime.XDecref(_pyType); + Runtime.XDecref(_pyValue); + // XXX Do we ever get TraceBack? // + if (_pyTB != IntPtr.Zero) + { + Runtime.XDecref(_pyTB); + } + } + finally + { + PythonEngine.ReleaseLock(gs); + } + } + catch + { + // Do nothing. + } + } + } + else + { + if (_pyType != IntPtr.Zero) + { + _referenceDecrementer?.ScheduleDecRef(_pyType); + } + + if (_pyValue != IntPtr.Zero) + { + _referenceDecrementer?.ScheduleDecRef(_pyValue); + } + if (_pyTB != IntPtr.Zero) { - Runtime.XDecref(_pyTB); + _referenceDecrementer?.ScheduleDecRef(_pyTB); } - PythonEngine.ReleaseLock(gs); } - GC.SuppressFinalize(this); - disposed = true; } } From 66fc91b9eb946e41d7c355bfa1fbbac73baedf3d Mon Sep 17 00:00:00 2001 From: dse Date: Tue, 5 Sep 2017 23:13:17 +0400 Subject: [PATCH 11/16] Added PYTHONNET_GC_DEBUG environment variable option for debugging GC related errors. --- CHANGELOG.md | 2 ++ src/runtime/pythonengine.cs | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a841ad5f5..4c2509d60 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,8 @@ This document follows the conventions laid out in [Keep a CHANGELOG][]. ### Fixed - Fixed memory leaks, caused by non-working PyObject, PythonException, DelecateManager->Dispatcher finalizers. + Set environment variable PYTHONNET_GC_DEBUG=1 to force full GC on each PythonEngine.Shutdown, this allows + to esily detect GC related bugs. - Fixed secondary PythonEngine.Initialize call, all sensitive static variables now reseted. This is a hidden bug. Once python cleaning up enough memory, objects from previous engine run becomes corrupted. - Fixed Visual Studio 2017 compat (#434) for setup.py diff --git a/src/runtime/pythonengine.cs b/src/runtime/pythonengine.cs index 7bbc842cf..d2a259a20 100644 --- a/src/runtime/pythonengine.cs +++ b/src/runtime/pythonengine.cs @@ -4,6 +4,7 @@ using System.Linq; using System.Reflection; using System.Runtime.InteropServices; +using System.Threading; namespace Python.Runtime { @@ -17,6 +18,12 @@ public class PythonEngine : IDisposable private static IntPtr _pythonHome = IntPtr.Zero; private static IntPtr _programName = IntPtr.Zero; private static IntPtr _pythonPath = IntPtr.Zero; + private static bool _isGcDebugEnabled; + + static PythonEngine() + { + _isGcDebugEnabled = !String.IsNullOrWhiteSpace(Environment.GetEnvironmentVariable("PYTHONNET_GC_DEBUG")); + } public PythonEngine() { @@ -298,9 +305,25 @@ public static void Shutdown() { if (initialized) { + + if (_isGcDebugEnabled) + { + GC.Collect(); + GC.WaitForPendingFinalizers(); + Thread.Sleep(100); + } + CurrentRefDecrementer?.Dispose(); CurrentRefDecrementer = null; + if (_isGcDebugEnabled) + { + GC.Collect(); + GC.WaitForPendingFinalizers(); + Thread.Sleep(100); + } + + PyScopeManager.Global.Clear(); // We should not release memory for variables that can be used without initialized python engine. // It's assumed that Py_GetPythonHome returns valid string without engine initialize. Py_GetPythonHome will always return the From 5649258fc6109e489f8277fbbb6ed0e770798061 Mon Sep 17 00:00:00 2001 From: dse Date: Wed, 6 Sep 2017 23:01:06 +0400 Subject: [PATCH 12/16] Removed unnecessary AllocHGlobal. --- src/runtime/interop.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/runtime/interop.cs b/src/runtime/interop.cs index 4ae4b61e0..872d5fc2c 100644 --- a/src/runtime/interop.cs +++ b/src/runtime/interop.cs @@ -352,7 +352,6 @@ static Interop() } keepAlive = new ArrayList(); - Marshal.AllocHGlobal(IntPtr.Size); pmap = new Hashtable(); pmap["tp_dealloc"] = p["DestructorFunc"]; From 7fd464be3a4455ec1ea56d4b6df63fbbcb0028aa Mon Sep 17 00:00:00 2001 From: dse Date: Sat, 2 Sep 2017 22:58:00 +0400 Subject: [PATCH 13/16] MethodBinding missing XIncref fix. (In the infrequent execution pass). --- src/runtime/methodbinding.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/runtime/methodbinding.cs b/src/runtime/methodbinding.cs index 07090a92c..b58c092de 100644 --- a/src/runtime/methodbinding.cs +++ b/src/runtime/methodbinding.cs @@ -21,11 +21,12 @@ public MethodBinding(MethodObject m, IntPtr target, IntPtr targetType) Runtime.XIncref(target); this.target = target; - Runtime.XIncref(targetType); if (targetType == IntPtr.Zero) { targetType = Runtime.PyObject_Type(target); } + + Runtime.XIncref(targetType); this.targetType = targetType; this.info = null; From 296d8ba3a77abddfa64f6edb1651af7b5a73d2f5 Mon Sep 17 00:00:00 2001 From: dse Date: Tue, 5 Sep 2017 22:52:44 +0400 Subject: [PATCH 14/16] Incref/Decref bugfix. --- src/embed_tests/TestPyAnsiString.cs | 1 + src/embed_tests/TestPyLong.cs | 1 + src/embed_tests/TestPyString.cs | 1 + 3 files changed, 3 insertions(+) diff --git a/src/embed_tests/TestPyAnsiString.cs b/src/embed_tests/TestPyAnsiString.cs index 9ba7d6cc6..b4a965ff7 100644 --- a/src/embed_tests/TestPyAnsiString.cs +++ b/src/embed_tests/TestPyAnsiString.cs @@ -63,6 +63,7 @@ public void TestCtorPtr() const string expected = "foo"; var t = new PyAnsiString(expected); + Runtime.Runtime.XIncref(t.Handle); var actual = new PyAnsiString(t.Handle); Assert.AreEqual(expected, actual.ToString()); diff --git a/src/embed_tests/TestPyLong.cs b/src/embed_tests/TestPyLong.cs index fe3e13ef5..903256f12 100644 --- a/src/embed_tests/TestPyLong.cs +++ b/src/embed_tests/TestPyLong.cs @@ -102,6 +102,7 @@ public void TestCtorDouble() public void TestCtorPtr() { var i = new PyLong(5); + Runtime.Runtime.XIncref(i.Handle); var a = new PyLong(i.Handle); Assert.AreEqual(5, a.ToInt32()); } diff --git a/src/embed_tests/TestPyString.cs b/src/embed_tests/TestPyString.cs index 9d1cdb0e9..0de436e35 100644 --- a/src/embed_tests/TestPyString.cs +++ b/src/embed_tests/TestPyString.cs @@ -64,6 +64,7 @@ public void TestCtorPtr() const string expected = "foo"; var t = new PyString(expected); + Runtime.Runtime.XIncref(t.Handle); var actual = new PyString(t.Handle); Assert.AreEqual(expected, actual.ToString()); From 6c1abfa4a9a3ab60f22cf4423fc95847270f80d4 Mon Sep 17 00:00:00 2001 From: dse Date: Thu, 7 Sep 2017 14:47:06 +0400 Subject: [PATCH 15/16] Stability tests improved, compile warning fixed. --- src/embed_tests/Program.cs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/embed_tests/Program.cs b/src/embed_tests/Program.cs index 03a752e89..a18894367 100644 --- a/src/embed_tests/Program.cs +++ b/src/embed_tests/Program.cs @@ -38,17 +38,16 @@ public static int Main(string[] args) dynamic os = PythonEngine.ImportModule("os"); os.environ["PATH"] = new PyString(pathEnv); } - } while (true); + } + while (!Console.KeyAvailable); return result; } - else - { - return new AutoRun(typeof(Program).Assembly).Execute( - args, - new ExtendedTextWrapper(Console.Out), - Console.In); - } + + return new AutoRun(typeof(Program).Assembly).Execute( + args, + new ExtendedTextWrapper(Console.Out), + Console.In); } } } From 80ff7549daf5ae530f80b6eac5f49565b61be2b7 Mon Sep 17 00:00:00 2001 From: dse Date: Wed, 13 Dec 2017 01:35:54 +0400 Subject: [PATCH 16/16] Finalizer implementation improvement. PythonException memory leak fix. Review fixes. --- src/embed_tests/TestFinalizer.cs | 62 ++++++++++++++++++ .../fixtures/MemoryLeakTest/pyraise.py | 8 +++ src/runtime/debughelper.cs | 6 +- src/runtime/pyiter.cs | 16 +++-- src/runtime/pyobject.cs | 31 ++++----- src/runtime/pyreferencedecrementer.cs | 8 +++ src/runtime/pyscope.cs | 41 ++++++------ src/runtime/pythonengine.cs | 2 +- src/runtime/pythonexception.cs | 63 +++++++++++-------- 9 files changed, 165 insertions(+), 72 deletions(-) create mode 100644 src/embed_tests/fixtures/MemoryLeakTest/pyraise.py diff --git a/src/embed_tests/TestFinalizer.cs b/src/embed_tests/TestFinalizer.cs index cfb8a491f..a7b6432a5 100644 --- a/src/embed_tests/TestFinalizer.cs +++ b/src/embed_tests/TestFinalizer.cs @@ -64,5 +64,67 @@ public void TestClrObjectFullRelease() Assert.IsFalse(weakRef.IsAlive, "Clr object should be collected."); } + + [Test] + [Ignore("For debug only")] + public void TestExceptionMemoryLeak() + { + dynamic pymodule; + PyObject gc; + dynamic testmethod; + + var ts = PythonEngine.BeginAllowThreads(); + IntPtr pylock = PythonEngine.AcquireLock(); + +#if NETCOREAPP + const string s = "../../fixtures"; +#else + const string s = "../fixtures"; +#endif + string testPath = Path.Combine(TestContext.CurrentContext.TestDirectory, s); + + IntPtr str = Runtime.Runtime.PyString_FromString(testPath); + IntPtr path = Runtime.Runtime.PySys_GetObject("path"); + Runtime.Runtime.PyList_Append(path, str); + + { + PyObject sys = PythonEngine.ImportModule("sys"); + gc = PythonEngine.ImportModule("gc"); + + pymodule = PythonEngine.ImportModule("MemoryLeakTest.pyraise"); + testmethod = pymodule.test_raise_exception; + } + + PythonEngine.ReleaseLock(pylock); + + double floatarg1 = 5.1f; + dynamic res = null; + { + for (int i = 1; i <= 10000000; i++) + { + using (Py.GIL()) + { + try + { + res = testmethod(Py.kw("number", floatarg1), Py.kw("astring", "bbc")); + } + catch (Exception e) + { + if (i % 10000 == 0) + { + TestContext.Progress.WriteLine(e.Message); + } + } + } + + if (i % 10000 == 0) + { + GC.Collect(); + } + } + } + + PythonEngine.EndAllowThreads(ts); + } } } diff --git a/src/embed_tests/fixtures/MemoryLeakTest/pyraise.py b/src/embed_tests/fixtures/MemoryLeakTest/pyraise.py new file mode 100644 index 000000000..444594283 --- /dev/null +++ b/src/embed_tests/fixtures/MemoryLeakTest/pyraise.py @@ -0,0 +1,8 @@ +def test_raise_exception(number=3, astring='abc'): + raise ValueError("testing for memory leak") + return astring * int(number) + +def test_raise_exception2(number, astring): + #raise ValueError("testing for memory leak") + #astring * int(number) + return "test" diff --git a/src/runtime/debughelper.cs b/src/runtime/debughelper.cs index 2a91a74b4..3fe9ee5bb 100644 --- a/src/runtime/debughelper.cs +++ b/src/runtime/debughelper.cs @@ -116,10 +116,10 @@ internal static void debug(string msg) Console.WriteLine(" {0}", msg); } - /// + /// /// Helper function to inspect/compare managed to native conversions. - /// Especially useful when debugging CustomMarshaler. - /// + /// Especially useful when debugging CustomMarshaler. + /// /// [Conditional("DEBUG")] public static void PrintHexBytes(byte[] bytes) diff --git a/src/runtime/pyiter.cs b/src/runtime/pyiter.cs index ee07bcecf..5fa6ec70c 100644 --- a/src/runtime/pyiter.cs +++ b/src/runtime/pyiter.cs @@ -40,14 +40,20 @@ public PyIter(PyObject iterable) } } - protected override void Dispose(bool disposing) + protected internal override void Dispose(bool disposing) { - if (null != _current) + try { - _current.Dispose(); - _current = null; + if (null != _current) + { + _current.Dispose(disposing); + _current = null; + } + } + finally + { + base.Dispose(disposing); } - base.Dispose(disposing); } public bool MoveNext() diff --git a/src/runtime/pyobject.cs b/src/runtime/pyobject.cs index 68878bf39..bfaf90168 100644 --- a/src/runtime/pyobject.cs +++ b/src/runtime/pyobject.cs @@ -132,39 +132,32 @@ public T As() /// to Python objects will not be released until a managed garbage /// collection occurs. /// - protected virtual void Dispose(bool disposing) + protected internal virtual void Dispose(bool disposing) { if (!disposed) { disposed = true; + IntPtr objToDispose = obj; + obj = IntPtr.Zero; if (disposing) { - try + if (Runtime.Py_IsInitialized() > 0 && !Runtime.IsFinalizing) { - if (Runtime.Py_IsInitialized() > 0 && !Runtime.IsFinalizing) + IntPtr gs = PythonEngine.AcquireLock(); + try { - IntPtr gs = PythonEngine.AcquireLock(); - try - { - Runtime.XDecref(obj); - obj = IntPtr.Zero; - } - finally - { - PythonEngine.ReleaseLock(gs); - } + Runtime.XDecref(objToDispose); + } + finally + { + PythonEngine.ReleaseLock(gs); } - } - catch - { - // Do nothing. } } else { - _referenceDecrementer?.ScheduleDecRef(obj); - obj = IntPtr.Zero; + _referenceDecrementer?.ScheduleDecRef(objToDispose); } } } diff --git a/src/runtime/pyreferencedecrementer.cs b/src/runtime/pyreferencedecrementer.cs index 26bbf6a00..674ebb5f9 100644 --- a/src/runtime/pyreferencedecrementer.cs +++ b/src/runtime/pyreferencedecrementer.cs @@ -7,6 +7,14 @@ namespace Python.Runtime { + /// + /// Background python objects reference decrementer. + /// !!! By some unknown reasons python references decrements have been performed in the GC thread - affects GC. !!! + /// This component contains background thread with the queue of IntPtr python object references. + /// Finalizers should schedule reference decrement operation in this component to avoid problem with GC. + /// Each PythonEngine Init/Shutdown have it's own PyReferenceDecrementer, so PyObject should also have a field with the + /// reference to the correct PyReferenceDecrementer. + /// internal class PyReferenceDecrementer : IDisposable { private static readonly DedicatedThreadTaskScheduler DedicatedThreadTaskScheduler = new DedicatedThreadTaskScheduler(); diff --git a/src/runtime/pyscope.cs b/src/runtime/pyscope.cs index 2b38e460d..88161e677 100644 --- a/src/runtime/pyscope.cs +++ b/src/runtime/pyscope.cs @@ -31,7 +31,7 @@ public class PyScope : DynamicObject, IDisposable /// /// the python Module object the scope associated with. /// - internal readonly IntPtr obj; + internal IntPtr obj; /// /// the variable dict of the scope. @@ -522,34 +522,37 @@ protected virtual void Dispose(bool disposing) { if (!_isDisposed) { - _isDisposed = true; - + IntPtr objToDispose = obj; if (disposing) { try { - if (Runtime.Py_IsInitialized() > 0 && !Runtime.IsFinalizing) - { - IntPtr gs = PythonEngine.AcquireLock(); - try - { - Runtime.XDecref(obj); - } - finally - { - PythonEngine.ReleaseLock(gs); - } - } + OnDispose?.Invoke(this); + } + finally + { + _isDisposed = true; + obj = IntPtr.Zero; } - catch + + if (Runtime.Py_IsInitialized() > 0 && !Runtime.IsFinalizing) { - // Do nothing. + IntPtr gs = PythonEngine.AcquireLock(); + try + { + Runtime.XDecref(objToDispose); + } + finally + { + PythonEngine.ReleaseLock(gs); + } } - OnDispose?.Invoke(this); } else { - _referenceDecrementer?.ScheduleDecRef(obj); + _isDisposed = true; + obj = IntPtr.Zero; + _referenceDecrementer?.ScheduleDecRef(objToDispose); } } } diff --git a/src/runtime/pythonengine.cs b/src/runtime/pythonengine.cs index d2a259a20..e0593140c 100644 --- a/src/runtime/pythonengine.cs +++ b/src/runtime/pythonengine.cs @@ -598,7 +598,7 @@ internal GILState() protected virtual void Dispose(bool disposing) { - //ReleeaseLock is thread bound and if it's called in finalizer thread it can wrongly release lock. + //ReleaseLock is thread bound and if it's called in finalizer thread it can wrongly release lock. if (disposing) { PythonEngine.ReleaseLock(state); diff --git a/src/runtime/pythonexception.cs b/src/runtime/pythonexception.cs index 3ce017851..2bff9021c 100644 --- a/src/runtime/pythonexception.cs +++ b/src/runtime/pythonexception.cs @@ -21,9 +21,11 @@ public PythonException() _referenceDecrementer = PythonEngine.CurrentRefDecrementer; IntPtr gs = PythonEngine.AcquireLock(); Runtime.PyErr_Fetch(ref _pyType, ref _pyValue, ref _pyTB); - Runtime.XIncref(_pyType); - Runtime.XIncref(_pyValue); - Runtime.XIncref(_pyTB); + + // Those references already owned by the caller + ////Runtime.XIncref(_pyType); + ////Runtime.XIncref(_pyValue); + ////Runtime.XIncref(_pyTB); if (_pyType != IntPtr.Zero && _pyValue != IntPtr.Zero) { string type; @@ -44,7 +46,13 @@ public PythonException() } if (_pyTB != IntPtr.Zero) { - PyObject tb_module = PythonEngine.ImportModule("traceback"); + + if (_tbModule == null) + { + _tbModule = PythonEngine.ImportModule("traceback"); + } + PyObject tb_module = _tbModule; + Runtime.XIncref(_pyTB); using (var pyTB = new PyObject(_pyTB)) { @@ -54,11 +62,14 @@ public PythonException() PythonEngine.ReleaseLock(gs); } + private static PyObject _tbModule; + // Ensure that encapsulated Python objects are decref'ed appropriately // when the managed exception wrapper is garbage-collected. ~PythonException() { + Dispose(false); } @@ -152,49 +163,51 @@ protected void Dispose(bool disposing) { disposed = true; + IntPtr pyTypeToDispose = _pyType; + _pyType = IntPtr.Zero; + + IntPtr pyValueToDispose = _pyValue; + _pyValue = IntPtr.Zero; + + IntPtr pyTBToDispose = _pyTB; + _pyTB = IntPtr.Zero; + if (disposing) { if (Runtime.Py_IsInitialized() > 0 && !Runtime.IsFinalizing) { + IntPtr gs = PythonEngine.AcquireLock(); try { - IntPtr gs = PythonEngine.AcquireLock(); - try - { - Runtime.XDecref(_pyType); - Runtime.XDecref(_pyValue); - // XXX Do we ever get TraceBack? // - if (_pyTB != IntPtr.Zero) - { - Runtime.XDecref(_pyTB); - } - } - finally + Runtime.XDecref(pyTypeToDispose); + Runtime.XDecref(pyValueToDispose); + // XXX Do we ever get TraceBack? // + if (pyTBToDispose != IntPtr.Zero) { - PythonEngine.ReleaseLock(gs); + Runtime.XDecref(pyTBToDispose); } } - catch + finally { - // Do nothing. + PythonEngine.ReleaseLock(gs); } } } else { - if (_pyType != IntPtr.Zero) + if (pyTypeToDispose != IntPtr.Zero) { - _referenceDecrementer?.ScheduleDecRef(_pyType); + _referenceDecrementer?.ScheduleDecRef(pyTypeToDispose); } - if (_pyValue != IntPtr.Zero) + if (pyValueToDispose != IntPtr.Zero) { - _referenceDecrementer?.ScheduleDecRef(_pyValue); + _referenceDecrementer?.ScheduleDecRef(pyValueToDispose); } - if (_pyTB != IntPtr.Zero) + if (pyTBToDispose != IntPtr.Zero) { - _referenceDecrementer?.ScheduleDecRef(_pyTB); + _referenceDecrementer?.ScheduleDecRef(pyTBToDispose); } } }