Re: thoughts about unwind_protect

[Jaroslav Hajek]

On Mon, Jan 4, 2010 at 8:17 AM, John W. Eaton <address@hidden> wrote:
> On  2-Jan-2010, Jaroslav Hajek wrote:
>
> | one problem I see about the unwind_protect mechanism is that there is
> | no automatic cleanup on function exit.
> | Usually a function code goes like this
> |
> | unwind_protect_t frame = unwind_protect::begin_frame ();
> |
> | unwind_protect::protect_var (my_var);
> | // do stuff
> |
> | unwind_protect::run_frame ();
> |
> | The problem is that the last line is not executed if an exception
> | occurs in the function body, so the unwind_protect operation would be
> | delayed until the next run_frame or run_all after catching the
> | exception. Such a delay could have disastrous consequences, however,
> | if local variables (which no longer exist) were somehow involved in
> | the registered cleanup action.
>
> I don't think local variables were intended to be handled by the
> unwind_protect mechanism.  Are there any cases where unwind_protect is
> being used for local variables?
>
> | To combat this problem, liboctave
> | introduces the octave_interrupt_hook, which is hooked to a handler
> | that doesn unwind_protect::run_all (). Hence, in case of interrupt
> | exception, the *whole* unwind_protect stack is run before the
> | exception is even thrown.
> | Usually, this does the right thing, since the exception then
> | propagates to the top level (main loop).
> | However, it is not advisable to catch interrupt exceptions (for
> | possible additional handling), because at that moment cleanup actions
> | will be performed for the whole call stack, so thing may not be in
> | feasible state (for instance, the octave_call_stack).
> |
> | what I suggest is to instead use this scheme:
> |
> | unwind_protect frame;
> |
> | frame.protect_var ();
> |
> | // do_stuff
> |
> | // frame.run () // can be done explicitly or at destruction time.
> |
> | This has a number of advantages, being simpler and cleaning up
> | automatically. The only disadvantage is that exceptions should not
> | occur in destructors, so if a cleanup action is registered that can
> | throw exception (which is very rare), that action should be run
> | explicitly (with possibly handling the exception).
> |
> | I think this is not too much of a hassle, especially considering that
> | in fact now each frame is run explicitly. After this change, I think
> | it may be possible to eliminate the octave_interrupt_hook.
>
> This seems like an improvement to me, so I'd say go ahead and make the
> changes.
>

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

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 ().

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.

I didn't yet do more thourough testing than a plain make check; I'll
try more later. In fact I'll be a bit surprised if everything will
work correctly from the start :)

Comments? Thoughts?

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