Uni 56832 another crash fix attempt #6
Conversation
This should be platform-independent: - define tp_alloc, tp_clear, and tp_is_gc in native code - don't try the dlclose trick after shutdown This passes the pythonnet EmbeddingTest suite, plus the hotReloadCrashRepro test. Unknown if it passes the Python.Test suite: I can't figure out how to run those even in the unmodified pythonnet.
Added MONO_OSX define. Switched from UCS4 to UCS2. Removed a commented-out section that is required in VS for mac (but apparently OK to omit in VS for windows).
|
Note: I've also tried just synthesizing 'return 0' and 'return 1' via allocating an executable page of memory and dumping opcodes in there. Works fine on mac, should work fine on all three 64-bit intel platforms. Higher risk than this hack for now. |
| // - tp_traverse(object, ...) returns 0 | ||
| // - tp_clear(object) returns 0 | ||
| // - tp_is_gc(type) returns non-zero | ||
| // In addition, these functions can get called after a domain reload, |
There was a problem hiding this comment.
Small detail but I think we should say "domain unload". Domain reload is kind of application-specific, it is not something that is part of the C# framework. In other words, it might be too "Unity-specific"
| { | ||
| var lib = NativeMethods.LoadLibrary(Runtime.PythonDLL); | ||
| var zeroslot = NativeMethods.GetProcAddress(lib, "PyAST_Check"); | ||
| var trueslot = NativeMethods.GetProcAddress(lib, "Py_GetVersion"); |
There was a problem hiding this comment.
What if the procs don't exist? I know it is unlikely, but should you catch an exception of some sort in that case?
Also, I think it would be good to make sure PyAST_Check returns 0 and Py_GetVersion returns non-zero. Is there a way you could add an assertion in Debug perhaps?
|
I did it much better over --> there #7 |
On a domain reload, some objects are leaked. When we reinitialize python, those leaked objects are still being tracked by gc. This means their types' tp_traverse, tp_clear, and tp_is_gc slots can get invoked. But since those are implemented in C#, and we unloaded the domain, those slots are pointing to garbage.
Our previous attempt was to unload the cpython library itself and reload it. This works on Windows, but does not work on linux and osx: any native modules that have been loaded depend on cpython, so those operating systems refuse to unload cpython.
This fix instead makes the slots point to functions that are still valid after reloading, but that have no side effects and return the same values (0 for tp_traverse and tp_clear; non-zero for tp_is_gc).
The function that returns zero is PyAST_Check. That means there's a latent bug if someone were to use C# to define a subtype of ast.AST (the python abstract syntax tree). We'd have to document that.
Very minor change embedded in this change: switch from Hashtable to HashSet for cleaner code (and perhaps some speedup).
TODO: