Add soft shutdown by amos402 · Pull Request #958 · pythonnet/pythonnet · GitHub

What does this implement/fix? Explain your changes.

An upgraded version for #754
An implementation for #957
Also included some minor fixes.

Still simple and roughly, reset all GC objects by a save state, the old domain objects will be leaking and unaccessible.
Here's my next plan, I think I can replace all the clr objects slots to solid callbacks, that may help the memory leaking.

How to use: use PythonEngine.Initialize(softShutdown: true) for startup.

Updated Details:

Changes from above: objects in GC chains will not remove by default, it can be setting by RuntimeState.ShouldRestoreObjects, and modules which not in sys.modules after just call initial CPython will be remove from sys.modules on soft shutdown mode.
Multiple fixes about memory leaks and reference count errors.
The heap types which represent managed types will degenerate to normal type objects by set their slot callbacks to some function that builtin CPython regardless of normal mode or soft shutdown mode. Before their degenerations, through using tp_travese and tp_clear to release their sub objects, then use SlotsHolder.ReleaseTypeSlots reset their slots.
Points can be optimize for the above mechanism:
- tp_travese and tp_clear are not necessary for all time, they will become overhead for CPython's GC, maybe we can just use C# methods instead.
- Maybe name of SlotsHolder is not suitable because it also handle the memory release.
tp_is_gc and some other slots are no need to be override at start, they can inherit from PyType_Type by default.
Interop do not holding the delegate objects by calling GetThunk anymore, the SlotsHolder will take care of them.
__import__ will restore at shutdown.

Does this close any currently open issues?

#957

Any other comments?

Checklist

Check all those that are applicable and complete.

Make sure to include one or more tests for your change
If an enhancement PR, please create docs and at best an example
Add yourself to AUTHORS
Updated the CHANGELOG

Can you explain how does soft shutdown work in more details?

The base concept is, after Py_Initialize, save all GC objects and modules, and reset them when you shutting down.
Since the domain will be unload, the saved state cannot use any C# data types, also we need a dummy GC for storing the old objects for make sure some dealloc operations can call _PyObject_GC_UnTrack macro without a nullptr error. You can see the details in runtime_state.cs.
After discuss with @mjmvisser, he points some problems that relate restart Python will crash the PyQt. So I try to make a solution for that. I was intend to create two modes, one for reset all GC objects and modules(for now) and one for reset all slots of CLR types after previous mode, but I just saw this issue(#957), so I push it in advance.
Well, although my computer pass all tests, but the CI still not, I still need some fixes for the CI.

I'm unsure how this relates to #957. Seems to be more about improving the GC handling?

The prototype that @BadSingleton did was, in implementation, fairly simple (took us a while to figure it out of course):

Don't Py_Finalize in Shutdown
On Shutdown, go through the TypeManager and set all the slots to zero (except the gc ones).

Well, Python crash is connect with GC handling, after domain unload, when the GC collect occurred, it will traverse all GC objects, but the memory from old domain has been invalid, any access will result to crash.
If you just take them from GC chains, you can avoid access those invalid memory access to prevent crash. It maybe simple, but lots of memory will leak. So I need to do a lot of work to reduce the leaking, but it still cannot avoid them at all.

* Keep delegate of Thunk in tp_dict * Remove useless code

* Fix ref error at PythoneEngine.With

…nObject) will not care 'u' on Python2.

…hutdown forcedly * Disable restore objects on default

This should be internal. Also, I don't see it used anywhere.

This method also needs a XML doc note, that it is not thread-safe.

Why internal? The class is a internal already, and as a getter, I prefer use public.
Hummm, seems I only use it in my previous version, I will check it latter.

Still need to address thread safety. If called from different threads this can cause reading stale values from memory.

AllocGCHandle(true) - specify the parameter name here - it is unclear what true means.
E.g. AllocGCHandle(owned: true)

Please, add a XML doc to both methods, mentioning, that neither of them affect refcounts.

Please, check the return values of these calls.

Also, wrap code, operating on objs and gc in try .. finally ...

Yes, checking return values is necessary although using these builtin functions will be failed by less probability.
But the try is not necessary, they're all C functions actually.

I've summarized my understanding in this document: https://docs.google.com/document/d/1a9OLsdKHXJ6MxHjo0WlVRcfjAiaP5hONNKUKKH6Tr-o

I see two PRs that would be easy to pull out of amos' code and check in right now:

The SlotsHolder, which implements the proposal in Python can't reliably re-initialize; new proposal for Unity's domain reload #957
The __import__ restoration.
They don't (shouldn't) break anything so we can merge them in.

I don't yet understand what the gc chains thing is trying to address. I also don't understand how amos handles the modules. I suspect they're related.

It seems like amos is turning off python gc for the proxy objects. If that works, we should just turn it all off and then we can eliminate the native code page. But that's yet another PR.

There seems to be some refcount fixes as well. If we can get those fixes in separate PRs that makes it easier to review.

* Drop C module dependency when getting _PyObject_NextNotImplemented * Exception details for SetNoSiteFlag

* Add exception helper * Fixed refcnt error in ExtensionType.FinalizeObject * Fixed typename leaking * Fix refcnt error by using `using`

…en-2

…mments-2 Soft shutdown review comments 2

This should not be changed. If a function is needed, that allows access to the raw pointer when it is null, it should be a separate function for clarity.

I'll add an unchecked version

That checking is not necessary, because the method already named DangerousXXX, why it even can't obtain a null pointer. If it needs some null validation, it should do by the invoker not the internal.

@amos402 if the check is missed by accident by the invoker, the bug would be much easier to trace if it fails at the first access rather than much later on, when the raw null pointer is actually dereferenced.

What I concern is the method conformed the single responsibility or not. It is unfit that a method like this can't obtain a null pointer. For example, std::shared_ptr<T>::get can illustrate this point.
Anyway, I don't want to argue about this, if you insist, just check it, I don't mind it.

I tend to agree with @amos402, a method prefixed with Dangerous should let me do dangerous things. However, I think this modification should go in another PR as it modifies the current behaviour, and adding a checked version would add up a lot of changes all over the place.

Probably not an issue in this PR, but static methods generally should be thread-safe.

Addressing this is not necessary for acceptance.

I think it's to avoid double initialization of the runtime. With domain reloads we can't rely anymore on Py_IsInitialized.

We should avoid compile-time checks, as we eventually want to produce a single .NET Standard DLL. Can you replace this with a runtime check for AppDomain unloading support.

Why can't this work with BorrowedReference directly?

It could, but it would not improve readability. While we do get a borrowed reference, we really do own this reference.

I am not so sure. It took some time to figure out, that in 73-80 capsule is borrowed, in 80-82 it is a new object, and that in 81 the reference to it is not being stolen, and required decref in 82.

Maybe extract clearing of existing clr_data into a method.

Right, I get what you are saying now. I'll factor out the clearing.

And other code review changes

Even if netstandard lacks (for now) the necessary APIs for domain (un)loading, it is still possible to (un)load domains via the Mono C runtime.

In Runtime_data.cs

…e pointer" We Don't support python 2 anymore, but the CI machines may still be using it to build.

Also adds implicit IntPtr conversion operators to simplify their use.

…mments-3 * Remove compile-time check on NETSTANDARD * Extract method `ClearCLRData`

@BadSingleton / @amos402 can you please merge from master?

* Fix dead lock occur by calling PyEval_RestoreThread without check * Ignore `CollectOnShutdown` temporarily

Codecov Report

Merging #958 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #958   +/-   ##
=======================================
  Coverage   86.25%   86.25%           
=======================================
  Files           1        1           
  Lines         291      291           
=======================================
  Hits          251      251           
  Misses         40       40

Flag	Coverage Δ
#setup_linux	64.94% <ø> (ø)
#setup_windows	72.50% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c79be84...8d00e4c. Read the comment docs.

Please, avoid leaving partial changes in the history.

It's not partial, it's a complete one, although it can't pass on all runtimes.

amos402 and others added 7 commits June 14, 2019 02:35

Drop LoadLibrary dependency

d1928dc

Explain for getting _PyObject_NextNotImplemented

f000e08

Update changelog and authors

f882400

Merge branch 'master' into drop-dlopen

b7715ee

Merge remote-tracking branch 'remotes/upstream/master' into drop-dlopen

41ac665

Merge branch 'master' into drop-dlopen

c6dae9e

Add soft shutdown

4e19a4f

amos402 added 9 commits September 19, 2019 23:48

* Fix refcnt error

657452e

* Keep delegate of Thunk in tp_dict * Remove useless code

Fixed leaking of tp_name

91f64b9

* Reset type slots

b07b844

* Fix ref error at PythoneEngine.With

Clear ExtensionType

7db724e

Del SlotsHolder from tp_dict when shutting down

2a88be4

Fix refcnt error of qualname

2940973

Since ClassBase not override tp_init for BaseException, repr(Exceptio…

d108913

…nObject) will not care 'u' on Python2.

* Use environment variable PYTHONNET_SOFT_SHUTDOWN to enable soft s…

cc9e7e5

…hutdown forcedly * Disable restore objects on default

Fixed deadlock on finalizer after shutdown

1cb8e8c

lostmsu reviewed Oct 23, 2019

View reviewed changes

benoithudson mentioned this pull request Nov 21, 2019

Split from PR 958: restoring the __import__ after shutdown. #993

Merged

4 tasks

benoithudson reviewed Nov 21, 2019

View reviewed changes

Comment thread src/runtime/importhook.cs Outdated

amos402 mentioned this pull request Nov 30, 2019

Transfer the ownership of method's thunk to caller(split from #958) #1003

Merged

4 tasks

amos402 added 2 commits December 1, 2019 21:32

Merge branch 'master' into drop-dlopen

653a263

* Load PyModuleType without LibraryLoader

b3e889b

* Drop C module dependency when getting _PyObject_NextNotImplemented * Exception details for SetNoSiteFlag

filmor pushed a commit that referenced this pull request Dec 2, 2019

Fix refcnt errors (split from #958 ) (#1001 )

5f56ebc

* Add exception helper * Fixed refcnt error in ExtensionType.FinalizeObject * Fixed typename leaking * Fix refcnt error by using `using`

amos402 added 2 commits December 15, 2019 02:26

Merge remote-tracking branch 'remotes/upstream/master' into drop-dlop…

04d6dfb

…en-2

Prevent exception override

5150e61

amos402 added 3 commits September 10, 2020 15:23

Revert local changes

fa89b48

Remove unused code

80a7644

Merge pull request #3 from Unity-Technologies/soft-shutdown-review-co…

12c0206

…mments-2 Soft shutdown review comments 2

lostmsu reviewed Sep 13, 2020

View reviewed changes

BadSingleton and others added 10 commits September 15, 2020 14:31

Adds an unchecked version to get a BorrowedReference pointer

6a3cfc8

And other code review changes

Remove compile-time check on NETSTANDARD

98da1fa

Even if netstandard lacks (for now) the necessary APIs for domain (un)loading, it is still possible to (un)load domains via the Mono C runtime.

fixup! Remove compile-time check on NETSTANDARD

d7d44e8

Factor out the clearing of clr_data

6aa75c5

In Runtime_data.cs

Partially Revert "Adds an unchecked version to get a BorrowedReferenc…

2343f89

…e pointer" We Don't support python 2 anymore, but the CI machines may still be using it to build.

More changes to use BorrowedReference and NewReference

cc6b8e4

Also adds implicit IntPtr conversion operators to simplify their use.

fixup! More changes to use BorrowedReference and NewReference

d5fcfa4

fixup! fixup! More changes to use BorrowedReference and NewReference

fa47957

Remove unused method

ff956e4

Merge pull request #4 from Unity-Technologies/soft-shutdown-review-co…

c7b134c

…mments-3 * Remove compile-time check on NETSTANDARD * Extract method `ClearCLRData`

amos402 added 6 commits September 30, 2020 09:45

Merge branch 'master' into soft-shutdown

97e61a5

Wait for full GC Complete

0b9d2c1

Release atexit manually

178cbc8

* Remove fromPython

3a17f36

* Fix dead lock occur by calling PyEval_RestoreThread without check * Ignore `CollectOnShutdown` temporarily

Merge branch 'master' into soft-shutdown

3203457

Merge branch 'master' into soft-shutdown

8d00e4c

lostmsu reviewed Oct 8, 2020

View reviewed changes

lostmsu approved these changes Oct 8, 2020

View reviewed changes

lostmsu merged commit 2972f49 into pythonnet:master Oct 8, 2020

amos402 mentioned this pull request Oct 9, 2020

Drop LoadLibrary #880

Closed

4 tasks

danabr mentioned this pull request Oct 12, 2020

Exception in finalizer of custom encoder #1256

Open

lostmsu mentioned this pull request Nov 12, 2020

Cannot run twice; Unity crashes after running more than once #701

Closed

akaimody123 mentioned this pull request May 1, 2021

unity3d crashes on the second run henon/pythonnet_netstandard#18

Closed

Zulex mentioned this pull request Mar 4, 2022

C# jagged arrays and lists make Unity crash the second time #1713

Open

+                      private static IntPtr GetObjectDict(IntPtr ob)
+                      {
+                          return Marshal.ReadIntPtr(ob, ObjectOffset.DictOffset(ob));
+                      }
+                      private static void SetObjectDict(IntPtr ob, IntPtr value)
+                      {
+                          Marshal.WriteIntPtr(ob, ObjectOffset.DictOffset(ob), value);
+                      }

+                          var gc = PyImport_ImportModule("gc");
+                          var get_objects = PyObject_GetAttrString(gc, "get_objects");
+                          var objs = PyObject_CallObject(get_objects, IntPtr.Zero);
+                          var length = PyList_Size(objs);

                       /// <summary>Gets a raw pointer to the Python object</summary>
-                      public IntPtr DangerousGetAddress()
-                          => this.IsNull ? throw new NullReferenceException() : this.pointer;
+                      public IntPtr DangerousGetAddress() => this.pointer;

+                          if (!FullGCCollect())
+                          {
+                              Assert.IsTrue(WaitForCollected(op, hash, 10000));
+                          }
+                          Assert.IsFalse(shortWeak.IsAlive);
+                          garbage = Finalizer.Instance.GetCollectedObjects();
+                          Assert.IsNotEmpty(garbage, "The garbage object should be collected");
+                          Assert.IsTrue(garbage.Any(r => ReferenceEquals(r.Target, longWeak.Target)),
+                              "Garbage should contains the collected object");

                           cache = new Dictionary<Type, ClassBase>(128);
                       }
+                      public static IList<ClassBase> GetManagedTypes()

                           IntPtr py = Runtime.PyType_GenericAlloc(tp, 0);
-                          GCHandle gc = GCHandle.Alloc(this);
+                          GCHandle gc = AllocGCHandle(true);

+                          if (_isInitialized)
+                          {
+                              return;
+                          }
+                          _isInitialized = true;

+                          // Initialize modules that depend on the runtime class.
+                          AssemblyManager.Initialize();
+              #if !NETSTANDARD

+                          Marshal.WriteIntPtr(mem, (IntPtr)ms.Length);
+                          Marshal.Copy(data, 0, mem + IntPtr.Size, (int)ms.Length);
+                          IntPtr capsule = PySys_GetObject("clr_data").DangerousGetAddress();

Add soft shutdown by amos402 · Pull Request #958 · pythonnet/pythonnet · GitHub

Uh oh!

Add soft shutdown - #958

Add soft shutdown#958lostmsu merged 163 commits intopythonnet:masterpythonnet/pythonnet:masterfrom amos402:soft-shutdownamos402/pythonnet:soft-shutdownCopy head branch name to clipboard

Conversation

amos402 commented Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this implement/fix? Explain your changes.

Updated Details:

Does this close any currently open issues?

Any other comments?

Checklist

Uh oh!

lostmsu commented Sep 17, 2019

Uh oh!

amos402 commented Sep 17, 2019

Uh oh!

benoithudson commented Sep 17, 2019

Uh oh!

amos402 commented Sep 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

benoithudson commented Nov 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lostmsu Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lostmsu commented Sep 29, 2020

Uh oh!

codecov-io commented Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Add soft shutdown#958
lostmsu merged 163 commits into
pythonnet:masterfrom
amos402:soft-shutdown

amos402 commented Sep 17, 2019 •
edited
Loading

benoithudson commented Nov 7, 2019 •
edited
Loading

lostmsu Sep 15, 2020 •
edited
Loading

codecov-io commented Oct 8, 2020 •
edited
Loading