[C++-sig] Proposal to improve Python exception handling in Boost.Python

[David Abrahams]

Frank Benkstein wrote:
> Hi,
> 
> David Abrahams wrote:
>> on Thu Jun 19 2008, Frank Benkstein <benkstein-AT-langer-emv.de> wrote:
>>> <code>
>>> using namespace boost::python;
>>>
>>> try {
>>>   object value = some_dict["key"];
>>>   ...
>>> } catch (exceptions::key_error &e) {
>>>   BOOST_ASSERT(e.args() == make_tuple("key"));
>>>   // Do actual exception handling
>>>   ...
>>> } catch (exceptions::base_exception &e) {
>>>   std::cerr << e.traceback << std::flush;
>>>   std::cerr << e << e.flush;
>>> }
>>> </code>
>>>
>>> Those exceptions could be automatically translated back to the original
>>> Python exceptions and the traceback restored (using PyErr_Restore).
>> Also good... but what is restoring the traceback for?  Doesn't the
>> traceback that's already there work?
> 
> I forgot to mention the main API difference between error_already_set
> and this new approach:  error_already_set is just a signal that the
> Python error indicator (exception type, value and the traceback) is set.
> In this approach I would fetch those three objects and clear the
> indicator.  The reason for this is that if you wrap
> Py_Initialize/Py_Finalize in a RAII-Style class an exception might cause
> Py_Finalize to be called.  Py_Finalize will terminate the application
> with a fatal error (calling abort()) if the error indicator is set.

I don't know if you're aware of this, but Boost.Python is currently
incompatible with the use of Py_Finalize

But regardless of that, I don't understand the relationship of what you
wrote just above to clearing the Python error indicator.

> Also if you get another Python exception while already handling one you
> might want to be able to decide which one to rethrow.  If you don't save
> the original traceback it is lost when the second exception is raised.

I see.

>>> If you (the Boost.Python developers) agree to this proposal I would
>>> like to submit patches implementing the mentioned functionality
>>> through the following steps:
>>>
>>> 1. Convert all sites throwing error_already_set or calling
>>>    throw_error_already_set to calls to handle_python_exception.
>>>    handle_python_exception would just call throw_error_already_set
>>>    itself.
>> We probably ought to package up this little gem, which is a little
>> bigger than throw_error_already_set, into a single function in the
>> library:
>>
>>     if (PyErr_Occurred())
>>         throw_error_already_set();
> 
> I don't understead, what you mean.  My plan was to ensure that
> error_already_set is only thrown through a single function that we have
> control over.  This function can later decide what to do:  throw
> error_already_set or handle the exception in a Boost.Python specific
> way.

All I was saying was that that single function probably ought to include
the PyErr_Occurred check

>>> 2. Provide a registry for functions to be called when a specific Python
>>>    exception is encountered.  
>>>    These functions get called with three
>>>    boost::python::object arguments, the exception type, value and the
>>>    traceback object when an exception is detected by
>>>    handle_python_exception.  As a fallback if no matching function is
>>>    found error_already_set would still be thrown.
>>> 3. Provide a new namespace boost::python::exceptions with C++ exceptions
>>>    that correspond to the Python standard exceptions[1] including
>>>    inheritance.  
>> Good... but boost::python::exceptions::Exception should be derived from
>> error_already_set for backward compatibility.  Or maybe just
>>
>>         typedef exceptions::Exception error_already_set;
> 
> To reiterate:
> 
>  - If you catch error_already_set the error indicator is set, it is not
>    safe to call Py_Finalize().
>  - If you catch exceptions::Exception the error indicator is not set, it
>    is safe to call Py_Finalize().

Okay... well, I really dislike the idea of having both things in the
library at once, and I was never really thrilled with error_already_set.
 We should think about how to replace it with your thing.  My guess is
that if the library starts throwing something not derived from
error_already_set, regardless of whether it leaves the Python error
there, it's going to break more code than if we changed the meaning of
error_already_set or "lied" by deriving your thing from it.  There will
be transition issues, so we should discuss deprecation, compile-time
checks, runtime checks, etc.

>>>    Each class should have a static boost::python::object
>>>    member "mapped_type" wrapping the Python type object.  
>> How about "python_type?"
> 
> I don't care that much about the name.  mapped_type was just to make it
> clear that this is the member that is used to find the C++ exception
> class belonging to a Python exception class.  Also it is just the base
> type because the actual exception can be a subclass.
> 
> I imagine the following procedure:
> 
> 1. Register all your C++-mapped-exception classes in a sorted map or
>    list somewhere in Boost.Python.  When looking at the "mapped_type"
>    member It is ensured that the subclasses come before their bases.
> 2. Whenever a Python exception is detected by Boost.Python (through a
>    call to handle_python_exception) go through the map or list and throw
>    the first C++ exception that matches.
> 
> mapped_type would act as some kind of key in the map when storing the
> C++ exception.  The actual Python exception type would act as the key
> when retrieving the C++ class.
> 
>>>    And instance members "type", "value" and "traceback" so it can be
>>>    set or restored as the Python error indicator using PyErr_Restore
>>>    or PyErr_SetObject if the traceback is None.
>> Do you really need to store the type in each instance?  Oh, maybe you
>> do; it could end up being a python exception derived from one of the
>> standard exceptions.  Then what's "mapped_type" (a.k.a. "python_type")
>> for?

Inquiring minds wanna know.

>>> 4. Register all exceptions from 3. with the mechanism from 2.  Remove
>>>    the fallback throwing error_already_set from handle_python_exception
>>>    because base_exception would match all unknown exceptions.
>> Why do we want "base_exception," and why have the fallback if you're
>> just going to remove it?
> 
> The four steps were meant to be incremental improvements (patches) so
> that Boost.Python stays functional between each step.  I though that
> this would make reviewing easier.

I think I'd prefer a single analysis and proposal of what should be
done.  We can talk about how to get there after I understand where we're
going.

-- 
Dave Abrahams
BoostPro Computing
http://www.boostpro.com