Re: thoughts about unwind_protect

[John W. Eaton]

On  6-Jan-2010, Jaroslav Hajek wrote:

| OK, changeset is upstream:
| http://hg.savannah.gnu.org/hgweb/octave/rev/2cd940306a06
| 
| Summary of changes (long):
| 
| unwind_protect is restructured, partially rewritten and simplified.
| Rather than a single global stack, local frames are created, like
| this:
| 
| unwind_protect frame;
| frame.protect_var (my_var); // Save my_var value and restore it on cleanup
| frame.add_fcn (my_cleanup, my_param); // Call void my_cleanup
| (my_param) on cleanup.
| 
| The local definition has the big fat advantage that the cleanup
| actions are performed automatically at end of current block, even if
| exited by return, break, continue, or exception (even goto). This
| allowed a lot of simplifications in the code; in particular, a number
| of "goto cleanup" constructs went away, replaced by simple returns.
| Also, by this change Octave is again one small step closer to being thread 
safe.
| 
| Another improvement is that the cleanup actions are performed along
| the normal function returns. So far, this has been done by
| octave_interrupt_hook being hooked to unwind_protect::run_all,
| performing everything prior to even throwing the interrupt exception.
| 
| As a result, it was unsafe to catch octave_interrupt_exception for
| further handling, because at that point we may be in a function B
| called by function A, but both B's and A's cleanup actions have
| already been performed.
| Besides, this meant that when Octave was embedded, some cleanup
| actions may not have been performed correctly if an exception escaped
| out of feval or eval_string.
| 
| Further, the cleanup actions will be performed (or at least attempted)
| even in the case when std::bad_alloc is generated by means other of
| octave_throw_bad_alloc. Previously, the cleanup would be bypassed in
| this case (no bad_alloc_hook).
| 
| The frame may also be run explicitly via "frame.run ()" or or
| discarded via "frame.discard ()". It is also possible to discard/run
| just the top entry or several entries:
| frame.run_top ()
| frame.discard_top ()
| frame.run_top (2) // equivalent to run_top () twice

This all seems good to me. Thanks for working on it.

| here's a possible gotcha; I was not sure whether frame.run () should
| correspond to unwind_protect::run (), which was the former equivalent
| of run_top) or unwind_protect::run_frame (frame), but in the end the
| latter seemed more logical. Similarly for discard.
| So, frame.run () runs the *whole* frame, not just the top entry. Ditto
| for discard ().

Seems OK to me since we still have the ability to run or discard just
the top element with the run_top and discard_top methods.

| Now for the pitfalls:
| Generally, the cleanup actions registered should not throw exceptions,
| because they may be and often will be called from the unwind_protect
| destructor. Exceptions thrown in destructors are painful stuff in C++
| and strongly discouraged. Actually, they're not forbidden per se, but
| if an exception occurs in a local object's destructor and another one
| is already pending, the program aborts according to C++ standard. For
| reasoning and more details, consult the C++ FAQ.
| 
| In most cases, this is true; however, there are a few tough spots. The
| most striking cases were the try/catch and unwind_protect handlers in
| pt-eval.cc; I solved these by explicit calling of the cleanup codes
| rather than inserting them into unwind_protect; in fact, this solution
| seems cleaner than the previous and global variables are no longer
| needed.
| 
| Note that interpreter-level errors (i.e. raising error_state) are OK;
| just unhandled interrupt and liboctave exceptions should not occur.
| 
| For cases where the exception safety is not clear, unwind_protect_safe
| is derived. This only differs in the destructor; cleanup actions are
| run in try/catch and *any* exception is swallowed and converted to an
| error message. This may mean that something is wrong anyway; but at
| least it will try to return to prompt rather than aborting the
| program. See the usages of unwind_protect_safe.
| 
| On the whole, however, if this ever happens, I think we should make
| the cleanup actions exception-safe instead of relying on this
| mechanism.

OK.

I don't know that I completely follow all of the above, but what are
the implications for possibly converting Octave's internal error
handling code from setting error_state to something that uses C++
exceptions instead?

jwe