Removed ShutdownMode. Now always behaves like original Reload
#1638
Conversation
This file contains 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
lostmsu
force-pushed
the
cleanup/ShutdownModes
branch
3 times, most recently
from
7ff467a
to
7ab2bfc
Compare
filmor
reviewed
Dec 20, 2021
lostmsu
force-pushed
the
cleanup/ShutdownModes
branch
from
7ab2bfc
to
fd451d5
Compare
|
@filmor looks like I got tests to pass, so this is ready. |
lostmsu
force-pushed
the
cleanup/ShutdownModes
branch
2 times, most recently
from
3157cf5
to
5113257
Compare
filmor
reviewed
Dec 21, 2021
filmor
reviewed
Dec 21, 2021
filmor
reviewed
Dec 21, 2021
| } | ||
| else | ||
| { | ||
| ResetPyMembers(); | ||
| if (mode != ShutdownMode.Extension) | ||
| { | ||
| Py_Finalize(); |
There was a problem hiding this comment.
This is my only real gripe with this: Embedders would not be able to finalise the Python runtime at all anymore. Maybe we should provide this (Shutdown() + Py_Finalize()) as a possible footgun with a long name?
There was a problem hiding this comment.
We can always add it if there is a strong request. Instead what happened is people opening bugs about crashes after restarting runtime because NumPy does not support it. I'd rather avoid it.
Moreover, the behavior of the whole thing when either of runtimes is shut down completely is pretty much undefined.
filmor
reviewed
Dec 21, 2021
lostmsu
force-pushed
the
cleanup/ShutdownModes
branch
2 times, most recently
from
8081238
to
8a093c2
Compare
… is an equivalent of `ShutdownMode.Reload` also in this change: - fixed Python derived types not being decrefed when an instance is deallocated - reduced time and amount of storage needed for runtime reload - removed circular reference loop between Type <-> ConstructorBinding(s) + exposed Runtime.TryCollectingGarbage
clearing GCHandle from an instance of Python derived type would drop the last reference to it, so it was destroyed without being removed from reflectedObjects collection
lostmsu
force-pushed
the
cleanup/ShutdownModes
branch
from
8a093c2
to
dfb87dc
Compare
|
@filmor I am waiting for an explicit approval or more comments |
filmor
approved these changes
Dec 25, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this implement/fix? Explain your changes.
This removes all shutdown modes, except
Reload(which you don't need to specify, because it is the only one left).This means Python C runtime is never actually shut down when .NET calls
PythonEngine.Shutdown(). Instead, anything from .NET exposed to Python beforeShutdownbecomes unavailable untilPythonEngine.Initialize()is called again (which can be done from a differentAppDomain).Any other comments?
Also in this change:
ConstructorBinding(s)Runtime.TryCollectingGarbageA review from @amos402 would be welcome