Uni 63112 hotreload crash test#7
Merged
benoithudson merged 11 commits intopythonnet-masterfrom Oct 2, 2018
Merged
Conversation
When this test is not ignored, we get a crash. The bug fixes are coming soon (there's three bugs).
Shut down python when the domain that loaded python reloads.
Also added comments explaining what the hashset is about.
1. Implement tp_traverse, tp_clear, and tp_is_gc in native code in memory that is *not* released upon domain reload. Effect: fixes the crash on domain reload. Side effect: leaks a page every domain reload. 2. Use python's platform package to determine what we're running on, so we can use the right mmap/mprotect or VirtualAlloc/VirtualProtect routines, the right flags for mmap, and (theoretically) the right native code. It should be easy to add another system, as long as it has the same kinds of functionality. 3. Enable the domain reload test. Added some unit tests for the new stuff. 4. Remove tp_traverse, tp_clear, and tp_is_gc where it was implemented. Note: I haven't actually tested on windows and linux yet, I'm checking in so I can do that easily.
Nothing major:
- python platform.machine() returns x86_64 on mac, and AMD64 on windows,
for the same platform. Parse them properly.
- Microsoft's runtime C# compiler wants language version 2.0, so no
$"{foo}" syntax in our debug printing.
- Microsoft has decided you can't catch SEGV even in a unit test, so
disable that test on windows.
Most of my time was spent trying to convince VS to see the unit tests.
lassond
reviewed
Oct 2, 2018
src/embed_tests/TestDomainReload.cs
Outdated
| /// At the time this test was written, there was a very annoying | ||
| /// seemingly random crash bug when integrating pythonnet into Unity. | ||
| /// | ||
| /// The repro steps we that David Lassonde, Viktoria Kovecses and |
lassond
reviewed
Oct 2, 2018
|
|
||
| Runtime.Shutdown(); | ||
|
|
||
| AppDomain.CurrentDomain.DomainUnload -= OnDomainUnload; |
Collaborator
There was a problem hiding this comment.
I remember I got crashes on my side when trying to remove the callback like that. I verified that the list of callbacks was fresh on init (did not contain OnDomainUnload).
Maybe something else was involved in the crash
Author
There was a problem hiding this comment.
I thought I'd just cherry-picked this part, perhaps I missed a commit?
Anyway, no crash at the moment, we can get more complicated if we need to in a future commit.
lassond
approved these changes
Oct 2, 2018
Collaborator
lassond
left a comment
There was a problem hiding this comment.
Good work!
It makes me wonder if it will ever be possible to handle gc correctly with Python.NET
vkovec
reviewed
Oct 2, 2018
src/embed_tests/TestTypeManager.cs
Outdated
| Assert.That(Marshal.ReadInt64(page), Is.EqualTo(17)); | ||
|
|
||
| // Mark it read-execute, now we can't write anymore. | ||
| // We can't actually test access protectoin under Windows, |
src/runtime/runtime.cs
Outdated
| }; | ||
|
|
||
| /// <summary> | ||
| /// Map lower-case version of the python machine name to the processor |
vkovec
approved these changes
Oct 2, 2018
mac mmap allows it, so just set it on both platforms.
…nity-Technologies/pythonnet into uni-63112-hotreload-crash-test
Author
|
I've tested linux; all systems go! |
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.
Note: this is going into a copy of the pythonnet master, not to the Unity-technologies master which has lots of bumbling about from our earlier phases.
This patch implements a fix to two crashes when integrating pythonnet into Unity3d.
On domain reload, shut down pythonnet. Otherwise there'll be a bunch of objects in python that point to functions implemented in C# code that isn't there anymore. In particular, the trick of replacing the import function means that on an import statement in the second domain, python is calling into deallocated memory.
Implement the garbage collection support in native code in memory that is not deallocated. Python's garbage collector keeps pointers to python objects that were allocated in the first domain and leaked on Py_Finalize. Python doesn't clear that list in Py_Initialize. This means it will try to collect these objects in the second domain. This led to a crash because garbage collection support for the python objects was implemented in C#, so on the first gc in the second domain, python was calling into deallocated memory.
Use python's platform module to determine the platform at runtime so we know what native code to build and how to allocate an executable page. I'm preferring to check at runtime rather than at compile time so we can get away from shipping a .net DLL per configuration.
Unit tests for domain reload, the platform module, and for allocating a page of memory.
Support is currently windows and mac, i386 and x86_64. Linux might work too, but I haven't tested yet. Unit tests will fail with a descriptive message on other platforms.