Re: thoughts about unwind_protect

[Jaroslav Hajek]

On Thu, Jan 21, 2010 at 8:34 PM, John W. Eaton <address@hidden> wrote:
> 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
>

I think that would be nice, but it's going to be lengthy. error_state
also has some minor advantages - you can do your cleanup before
reacting to the error, but it's all doable with unwind_protect, I
think. If I have the time, I'd like to first focus at using a
hierarchy of exceptions in liboctave, instead of hardwired error
messages. The biggest problem, I think, is to propagate the exceptions
through Fortran callbacks (LSODE et al.).

-- 
RNDr. Jaroslav Hajek, PhD
computing expert & GNU Octave developer
Aeronautical Research and Test Institute (VZLU)
Prague, Czech Republic
url: www.highegg.matfyz.cz