[Lldb-commits] [PATCH] Fixup some issues with Python shutdown / finalization

[Enrico Granata]

> On Aug 7, 2014, at 7:01 PM, Zachary Turner <zturner at google.com> wrote:
> 
> I think you can reproduce these issues on MacOSX, you just need to build a debug version of python yourself.  The difference is that on Windows, we _have_ to build with a debug version of python.
> 

That’s a good idea, yes. Definitely worth a shot

> I will try your suggested change of wrapping the calls to Release inside of an if (!Py_IsInitialized()).  

If we are going to go down this route, maybe the safe way to do this is rewrite Reset() such that:

Reset (other = nullptr) {
 if (Py_Initialized()) { decref(current) }
 object = other
 if (Py_Initialized()) { incref(object) }
}

Then one could just say

m_session_dict.Reset(nullptr)
m_sys_modules_dict.Reset(nullptr)

If we are past a Py_Finalize(), then you will get the non-decref’ing behavior, otherwise the right thing will happen

IMHO, it is much cleaner - and safer - than exposing a Release() API. I’d really really prefer to avoid such a backdoorish API entirely, if at all feasible!
As an even stronger improvement, the destructor for PythonRefCountedObject might be moved from a plain decref() to a Reset(nullptr); then one wouldn’t even need the manual Reset() calls in the interpreter destructor - the destructors chain would just get it right all the time - which is A Good Thing
I am not sure if there was any reason to not do that (have the RefCountedObject destructor call Reset instead of manually decref'ing), so that might not be feasible after all, but worth a shot

> 
> You might be right about the atexit handlers, and perhaps with the other two issues fixed the atexit handlers can go back in.  Still, I'm not really a fan of atexit handlers in general, and I think it makes for more readable code if you can follow the sequence directly.  Maybe python atexit is more lenient than C atexit handlers, which are notorious for restricting what you can do inside them and perhaps why I'm wary of anything with the name "atexit"
> 

If atexit() is genuinely a cause of issues, sure we can stop using it. I guess my concern about this is not so much dotest.py per se - sure we can rework that - but I’d rather much not have limitations on what people can do when using the SB API - imagine a situation where someone is using LLDB’s API as well as some other Python library that chooses to use atexit() - if using LLDB’s API can’t coexist with atexit() now you’ve suddenly got a problem, something you have to document, and that can bite people in surprising ways
My preference here would be to fix the implementation of LLDB such that atexit() just works. A discussion on whether we’d like to use it in our test suite driver, or we prefer some other design, is orthogonal to that.

> 
> On Thu, Aug 7, 2014 at 5:39 PM, Enrico Granata <egranata at apple.com <mailto:egranata at apple.com>> wrote:
> Hi there,
> I have glanced over your patch
> I do not have a Windows box to try and repro these shutdown issues, so I might very well be missing some critical Windows-specific detail in my feedback
> 
> Release() seems to be a fairly dangerous accessor - indeed I am not positive that releasing without deallocation the session and modules dictionaries will do the right thing when Py_Finalize() is not involved
> I might very well create an SBDebugger (which - via the command interpreter - owns the ScriptInterpreterPython); if I simply let go of the last reference to the SBDebugger, the destruction chain should do the right thing, and I expect your release-without-decref calls might cause some objects to survive their due lifetime
> At the very least, would it be possible to wrap those releases in an if (Py_IsInitialized()) like you do in another change?
> 
> As for 3 - nothing in the Python documentation specifies that the atexit facility does not allow any “meaningful code” execution; the main caveat I could find online about it is that there are escape routes out of Python that atexit() does not handle - but that seems a very different concern from what you are seeing
> Even if we actually end up deciding that atexit is not to be used, your patch as-is has eliminated the call to the code to delete crashinfo.so - that is not critical of course, but it worries me to see a duplication of lldb.SBDebugger.Terminate() calls - it might be more appropriate, if anything, to define a function
> 
> def exitFromTestSuite(exitCode = None):
>  deleteCrashInfoDylib(dylib_dst)
>  lldb.SBDebugger.Terminate()
>  if exitCode != None:
>     sys.exit(exitCode)
> 
> Then we would all know to put our atexit calls in exitFromTestSuite(), and we’d have some degree of confidence that all cleanup tasks will be executed on all ways out of the test suite.
> But, again, I am not sure why atexit is giving you woes - that would probably be worth looking into
> 
> 
>> On Aug 7, 2014, at 4:03 PM, Zachary Turner <zturner at google.com <mailto:zturner at google.com>> wrote:
>> 
>> There were a number of issues related to python shutdown.  These issues appear to be present on every platform, but are benign except for Windows.  On Windows they present because Windows must build against a debug python interpreter, which asserts when these issues arise.  The issues, along with their respective fixes, are:
>> 
>> 1) decrementing the reference count of dead objects.  This happens because we store the session dictionary in the ScriptInterpreterPython class, and when these are destroyed on shutdown, they get decref'ed.  However, this happens during or after a Py_Finalize, so Python has already destroyed these objects.  The fix employed here is to add a Release() method to PythonObject() which simply drops the underlying object.
>> 
>> 2) Attempting to execute embedded python during Py_Finalize.  This results in error messages "NameError: module 'lldb' not found" after tests run.  The fix here is to only execute these commands if Py_IsInitialized()
>> 
>> 3) Use of sys.atexit().  atexit handlers are run as part of Py_Finalize, and at this point it is unspecified which, if any modules have been destroyed.  So no meaningful code can run here.  The fix employed here is to explicitly call lldb.SBDebugger.Terminate() before calling sys.exit().  A better fix would be to not use sys.exit() at all, but instead have a clean shutdown path, perhaps by way of raising an exception that is caught at the top-level of the python script.
>> 
>> http://reviews.llvm.org/D4826 <http://reviews.llvm.org/D4826>
>> 
>> Files:
>>  include/lldb/Interpreter/PythonDataObjects.h
>>  source/Interpreter/ScriptInterpreterPython.cpp
>>  test/dotest.py
>> <D4826.12296.patch>_______________________________________________
>> lldb-commits mailing list
>> lldb-commits at cs.uiuc.edu <mailto:lldb-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits <http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits>
> 
> - Enrico
> 📩 egranata@.com ☎️ 27683
> 
> 
> 
> 

- Enrico
📩 egranata@.com ☎️ 27683



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140807/be908f1e/attachment.html>