17

Many people are no doubt familiar with Mr. Alexandrescus ScopeGuard template (now part of Loki) and the new version ScopeGuard11 presented here: http://channel9.msdn.com/Shows/Going+Deep/C-and-Beyond-2012-Andrei-Alexandrescu-Systematic-Error-Handling-in-C

with source here: https://gist.github.com/KindDragon/4650442

In his talk at c++ and beyond 2012 he mentioned that he couldn't find a way to correctly detect if scope was being exited because of an exception. Therefore he couldn't implement a SCOPE_FAIL macro which would execute the supplied lambda (usually used for roll back code) if and only if the scope exited because of an exception. This would render the dismiss() member function unneeded and make code more readable.

Since I am by no means as genius or experienced as Mr. Alexandrescu I expect implementing SCOPE_FAIL is not as easy as this:

~ScopeGuard11(){                      //destructor
    if(std::uncaught_exception()){    //if we are exiting because of an exception
        f_();                         //execute the functor
    }
    //otherwise do nothing
}

My question is why not?

markalex
  • 8,623
  • 2
  • 7
  • 32
odinthenerd
  • 5,422
  • 1
  • 32
  • 61
  • It's strange, something tells me it should work, but if I try it, `uncaught_exception()` always returns `false`. – Andy Prowl Feb 17 '13 at 23:35
  • I vaguely remember Herb Sutter having something like this on GotW way back when but I can't find it any more. Maybe Alzheimer ;) or I'm not googleing the right thing. – odinthenerd Feb 17 '13 at 23:37
  • I think in the scope guard case, you could actually use `std::uncaught_exception`, since the scope guard will never be a member of another class (and surely not a local variable in some class' destructor). – Xeo Feb 17 '13 at 23:40
  • @Xeo: Still `std::uncaught_exception()` [seems to return `false` all the time](http://liveworkspace.org/code/qdJN6$0). Is it perhaps a bug, or am I overlooking something? – Andy Prowl Feb 17 '13 at 23:46
  • I haven't seen the talk -- how does SCOPE_FAIL relate to the code you posted? – Kerrek SB Feb 17 '13 at 23:53
  • @Xeo: Jeez. I suck so hard. – Andy Prowl Feb 18 '13 at 00:06
  • 2
    @PorkyBrain: http://www.gotw.ca/gotw/047.htm ? – Lightness Races in Orbit Feb 18 '13 at 00:12
  • @AndyProwl You can't use it in ```catch``` block since the exception is already caught. You can only use it in destructor. – JiaHao Xu Oct 08 '18 at 23:22

1 Answers1

12

With a ScopeGuard11 class that has your destructor, the member f_ may be called, even if it is not the current scope (that is supposed to be protected by the guard) that is being exited due to an exception. Use of this guard is unsafe in code that might be used during exception cleanup.

Try this example:

#include <exception>
#include <iostream>
#include <string>

// simplified ScopeGuard11
template <class Fun>
struct ScopeGuard11 {
     Fun f_;
     ScopeGuard11(Fun f) : f_(f) {}
     ~ScopeGuard11(){                      //destructor
        if(std::uncaught_exception()){    //if we are exiting because of an exception
            f_();                         //execute the functor
         }
         //otherwise do nothing
      }
};

void rollback() {
  std::cout << "Rolling back everything\n";
}
void could_throw(bool doit) {
  if (doit) throw std::string("Too bad");
}

void foo() {
   ScopeGuard11<void (*)()> rollback_on_exception(rollback);
   could_throw(false);
   // should never see a rollback here
   // as could throw won't throw with this argument
   // in reality there might sometimes be exceptions
   // but here we care about the case where there is none 
}

struct Bar {
   ~Bar() {
      // to cleanup is to foo
      // and never throw from d'tor
      try { foo(); } catch (...) {}
   }
};

void baz() {
   Bar bar;
   ScopeGuard11<void (*)()> more_rollback_on_exception(rollback);
   could_throw(true);
}

int main() try {
   baz();
} catch (std::string & e) {
   std::cout << "caught: " << e << std::endl;
}

You'd want to see one rollback when leaving baz, but you'll see two - including a spurious one from leaving foo.

JoergB
  • 4,383
  • 21
  • 19
  • 1
    Interesting. I'm wondering though if it is not a matter of good design to avoid this pattern. Here, `foo()` won't ever throw (otherwise it would be bad to call it from a destructor). But if it won't ever throw, why using a rollback on exception? – Andy Prowl Feb 18 '13 at 00:46
  • That `foo()` was set up to never throw, was for the sake of demonstration. In a real problem case, the conditions when `could_throw` could throw (sic!) would be less obvious. In that case the `Bar` destructor could do `try { foo(); } catch (...) {}`. Or there could be more layers in between, taking care that any exception in `foo()` would not leave the `Bar` destructor. Nevertheless you'd get a spurious rollback in `foo()` even if it is left by regular return. – JoergB Feb 18 '13 at 00:51
  • @AndyProwl: I have adjusted the example to make this clearer. – JoergB Feb 18 '13 at 00:54
  • Makes sense. However, in that case the constructor of `ScopeGuard11` could check whether `uncaught_exception()` returns `true` and, in that case, save the result of `std::current_exception()`. In the destructor, it would then invoke `std::current_exception()` again and compare the result with the previously stored exception pointer. If equal, it won't do anything. If different, it will invoke the rollback function. Does it make sense? – Andy Prowl Feb 18 '13 at 01:03
  • OK, please forget that, it won't work: `std::current_exception()` will return `nullptr` before the exception is handled. +1ed – Andy Prowl Feb 18 '13 at 01:15
  • I can see that dividing code into "can or can't be run as part of cleanup" is problematic. I assume there is no way to enforce a "this can't be called from a destructor" policy syntactically? Like a InDestructor flag or something? – odinthenerd Feb 18 '13 at 10:35
  • @AndyProwl Could that be made to work if `uncaught_exception` returned `exception_ptr` instead of `bool`? ISTM that would be a compatible change; would it be feasible? – ecatmur Feb 18 '13 at 11:27
  • 1
    @ecatmur: Probably yes, but it would require a change to `current_exception()` as well. Currently, `current_exception()` returns `nullptr` before control is transferred to an exception handler. I'm still thinking, though, that the OP's solution is acceptable if one takes proper care of not invoking functions with an exception guard while an exception has been thrown and not yet handled. I would say the constructor of `ScopeGuard11` may even assert (or throw an exception itself) if `uncaught_exception()` returns `true`. But I'm just thinking aloud, not sure if it's a good idea. – Andy Prowl Feb 18 '13 at 12:11
  • @AndyProwl That would require even larger changes to how exception support operates. `current_exception` may make a fresh copy of the "internal" exception every time, so you can't rely on exception object identity at all. – JoergB Feb 18 '13 at 12:57
  • @JoergB: Correct. 18.8.5/8: *"It is unspecified whether the return values of two successive calls to current_exception refer to the same exception object. [ Note: That is, it is unspecified whether current_exception creates a new copy each time it is called. —end note ]"* – Andy Prowl Feb 18 '13 at 13:00
  • @AndyProwl an asserting constructor of scope guard is something I also thought of, the problem is that the assert would only show its self if the destructor is actually called because of stack unwinding, if an exception is very rarely thrown in that context an error (or misuse) could stay undetected for a very long time. – odinthenerd Feb 18 '13 at 14:07
  • @AndyProwl I think this class would work if, on construction, one checks `uncaught_exception() == false`. If it returns `true`, I'm unsure what one should do... `throw std::logic_error( "Exception unwinding is active!" );`? – Charles L Wilcox Mar 27 '13 at 14:44
  • @AndyProwl Not to self promote, but I proposed this as a possible solution to my own question in [Question 15504166](http://stackoverflow.com/questions/15504166/how-should-one-log-when-an-exception-is-triggered). – Charles L Wilcox Mar 27 '13 at 14:47