4

Recently, I came across an issue about the design of scope guard. A scope guard invokes a supplied function object (usually performs cleanup procedures) upon exiting the enclosing scope. The current design is as follows:

#define HHXX_ON_SCOPE_EXIT_F(...) ...
#define HHXX_ON_SCOPE_EXIT(...) HHXX_ON_SCOPE_EXIT_F([&]() { __VA_ARGS__ })

HHXX_ON_SCOPE_EXIT_F(...) executes the function object as defined by __VA_ARGS__ upon exiting the enclosing scope. For example,

int i = 0;
{
  HHXX_ON_SCOPE_EXIT_F([&]() {
    assert(++i == 4);
  });
  HHXX_ON_SCOPE_EXIT_F([&]() {
    assert(++i == 3);
  });
  HHXX_ON_SCOPE_EXIT(
    assert(++i == 2);
  );
  HHXX_ON_SCOPE_EXIT(
    assert(++i == 1);
  );
}
assert(i == 4);

The implementation is also simple:

#define HHXX_ON_SCOPE_EXIT_F(...)                                              \
  auto HHXX_UNIQUE_NAME(hhxx_scope_guard) =                                    \
    make_scope_guard(__VA_ARGS__)

#define HHXX_ON_SCOPE_EXIT(...) HHXX_ON_SCOPE_EXIT_F([&]() { __VA_ARGS__ })

template <typename F>
class scope_guard {
public:
  explicit scope_guard(F f)
      : f_(std::move(f)) {
    // nop
  }
  ~scope_guard() {
    f_();
  }

private:
  F f_;
};

template <typename F>
auto make_scope_guard(F f) {
  return scope_guard<F>(std::move(f));
}

As you can see, it doesn't provide built-in support for a method to abandon the invocation at scope exit. Providing such a feature, however, invalidates conciseness and simplicity of the current design. It also induces certain performance overhead. Basically, I'm in favor of maintaining the current design and rejecting the feature proposal. I want to hear opinions from you, so as to be sure I'm making a right decision.

Glorfindel
  • 3,137
  • 6
  • 25
  • 33
Lingxi
  • 155
  • 1
  • 7
  • This would be better if you put the relevant code inline. Who knows how long that page will be in the same state as it is now? – Brandin Jan 29 '16 at 10:40
  • 1
    @Brandin Thanks for the mention. Question updated. – Lingxi Jan 29 '16 at 11:20
  • 3
    1) Do you know of any motivating use cases for the ability to "disable" a scope guard? I'm having trouble thinking of any, other than pathological examples that would indicate a scope guard wasn't the appropriate tool to begin with. 2) What prevents the user from simply passing a lambda that starts with `if(skipCleanup) { return;}`? (edit: just noticed you raised this point yourself on the github issue) – Ixrec Jan 29 '16 at 11:22
  • @lxrec Yeah. That's exactly what I'm thinking about. More confident in making a rejection now :-) – Lingxi Jan 29 '16 at 11:26
  • 2
    Oh, I dunno... I find myself using `boost::mutex::scoped_lock::unlock()` from time to time, and it doesn't feel weird or wrong and it's not that unclear. Depends what code you're writing, and as a library function it may be worth providing this feature for flexibility. Though scoped-locks are pretty specific; I'm not sure that a feature for a general-purpose scope guard wouldn't just be inviting much more confusing code. – Lightness Races in Orbit Jan 29 '16 at 11:28
  • @LightnessRacesinOrbit It's nice to see you also been here :-) Guess I will just provide another scope guard class so as to provide the best of both worlds. – Lingxi Jan 29 '16 at 11:34
  • @Lingxi Your `scope_guard` depends on never getting copy- or move- assigned or constructed, the compiler always choosing to take advantage of RVO. If it doesn't, the payload gets called too often... – Deduplicator Jan 29 '16 at 13:20
  • @Deduplicator In my library, the `scope_guard` class template is not part of the interface and is put in namespace `detail` (see [here](https://github.com/Lingxi-Li/Happy_Hacking_CXX/blob/master/hhxx/scope_guard.hpp#L22)). Users are supposed to use the macros instead. In that case, the created guard objects are not manipulatable by the users. Besides, within `make_scope_guard()`, even if RVO does not happen, the object will be moved at least. No copy will ever happen. This is guaranteed by the standard. – Lingxi Jan 29 '16 at 13:27
  • 1
    Moving doesn't save you though, because the payload will still be called. – Deduplicator Jan 29 '16 at 13:28
  • @Deduplicator I think you are right. Do you think we can rely on RVO here? `make_scope_guard()` is simple, and we can mark it `inline`. – Lingxi Jan 29 '16 at 13:32
  • @Lingxi: There is no reason to take the chance: If you can rely on RVO, you can also rely on the compiler to note that the "thistimeforreal"-flag is always true. And while doing the former and being disappointed would uncover a horrible bug, the latter at most leads to a miniscule performance-degradation. – Deduplicator Jan 29 '16 at 13:34

2 Answers2

5

Your scope guard has interesting behaviour, and a quick code review could find various issues or possible issues (using the macro takes more code than not using the macro; your macro is flawed because it is a macro and is unnecessary; the gensym macro can't be used two times on the same line, which could happen in macros; many types are not move-constructible; many types are not copyable; you make a copy, then move the copy; I cannot lend a reference of a function to the guard because it creates a copy; ideally, the used callback would be nothrow; …).

You have correctly seen that disabling the guard can be trivially implemented by a user. An implementation that wants to be minimal and composable does not have to offer convenience features.

However, convenience features are mightily convenient, and make the difference between a correct implementation, and a useful implementation. Note that if you implement your guard in terms of std::function or in terms of a std::unique_ptr, it is trivial to support disabling the function without additional complication.

The last time I used such a feature was when I fork()ed a process and only wanted the guard to execute in the parent process, e.g. a guard to log a scope exit. Logging is a glorious use of guards.

However, guards are extremely useful in another circumstance: if I only want to handle exceptional cases, e.g. to roll back a transaction if anything goes wrong:

{
  Transaction transaction {};
  scope_guard rollbackGuard([&](){ transaction.rollback(); });
  ... // do things that could go wrong
  rollbackGuard.disarm();
  transaction.commit();
}

(note the use of a guard name to make the code more self documenting. Gensyms suck).

This would typically be written with a try-catch instead:

{
  Transaction transaction {};
  try { ... } // do things that could go wrong
  catch (...) { transaction.rollback(); throw; }
  transaction.commit();
}

Please understand Transaction as a symbol for some computation; if it were a singular object it should of course manage the rollback in its own destructor unless the commit succeeded.

The big advantage of guards over try–catch–finally is that a guard allows me to declare the on-exit code close to the data it interacts with, rather than 100 lines further down in a catch. If you allow your guards to be dismissed, your scope guards not only support the equivalent of try–finally, but also try–catch-with-rethrow. I would strongly suggest you go down this route, since it adds major convenience to users, with only minor impact on your implementation.

amon
  • 132,749
  • 27
  • 279
  • 375
  • Thanks for the detailed review. I can't think of a meaningful use-case where the macro may be used multiple times on a single line. Could you give an example? Concerning non-movable and non-copyable object, you could capture it by reference in a lambda. I could use `std::function/std::unique_ptr`, but that hampers performance. I think I will provide a class with disarm support in addition to the macros. – Lingxi Jan 29 '16 at 12:39
  • For use-cases where disarm is not needed, the name for a guard object doesn't make much sense. The code to execute at scope exit is documenting itself. – Lingxi Jan 29 '16 at 12:47
  • Your `scope_guard` does type-erasure? – Deduplicator Jan 29 '16 at 13:01
  • @Deduplicator No, it doesn't. – Lingxi Jan 29 '16 at 13:02
  • How can it accept just any lambda then? – Deduplicator Jan 29 '16 at 13:02
  • `scope_guard` is a class template parameterized on the function object type. – Lingxi Jan 29 '16 at 13:04
  • Yes, I get that. But in `scope_guard rollbackGuard([&](){ transaction.rollback(); });`, you don't select the proper instantiation. – Deduplicator Jan 29 '16 at 13:06
  • Oh, that's just an example pseudo-code of @amon. Please see the [`make_scope_guard()`](https://github.com/Lingxi-Li/Happy_Hacking_CXX/blob/master/hhxx/scope_guard.hpp#L40) helper function. – Lingxi Jan 29 '16 at 13:13
  • @Deduplicator yes, just assume this `scope_guard` takes a `std::function ` which does the type erasure for us. Otherwise, we could write `auto rollbackGuard = make_scope_guard(...)` to avoid an explicit template parameter, but those are unnecessary details. The important part is that disarming a guard (and names for guard objects) can be useful. – amon Jan 29 '16 at 13:14
  • Well, that scope-guard-maker depends on the scope-guard never being move- or copy-constructed, the compiler *always* opting for RVO. That's not guaranteed though, is it? (Hm, that was the OP's code...) – Deduplicator Jan 29 '16 at 13:15
  • 1
    @Deduplicator Event if RVO does not happen, it is moved at least (since C++11). No copy will ever happen. But it would be an issue for user-named guard objects that may be moved after construction. I overlooked that issue. Thanks for the catch :-) – Lingxi Jan 29 '16 at 13:22
  • _The big advantage of guards over try–catch–finally is that a guard allows me to declare the on-exit code close to the data it interacts with._ Which you can also do by writing a lambda function that is called in the catch. – D Drmmr Feb 11 '16 at 13:44
  • @DDrmmr consider scoping. Expressing this with try-catch would be extremely tedious: `{ maybeThrow(); Resource r; scope_guard resourceGuard([&](){ r.frobnicate(); }); maybeThrow(); }`. I believe that abstractions have to be *convenient*, or they won't be used. – amon Feb 11 '16 at 13:59
4

Well, there are different types of scope-guards you could create, depending on what features you need:

  1. Can it be disarmed? That one is free, something along those lines has to be implemented anyway. One cannot depend on RVO always coming to the rescue.
  2. Shall it only execute if an exception is thrown? This needs C++17, unfortunately.

In any way, there is already a class fitting the bill: std::unique_ptr.
Really, it just needs a simple convenience-function for ease of use.

For simplicity I used C++14 automatic return-type-deduction:

template<class F>
auto finally(F f) noexcept(noexcept(F(std::move(f)))) {
    auto x = [f = std::move(f)](void*){ f(); };
    return std::unique_ptr<void, decltype(x)>((void*)1, std::move(x));
}

auto guard = finally([&]{ final_action(); });

With C++17, one can restrict it to only execute when an exception got thrown:

template<class F>
[[nodiscard]] auto on_throw(F f) noexcept(noexcept(F(std::move(f)))) {
    auto x = [f = std::move(f)](void* p){
        if((intptr_t)p > std::uncaught_exceptions()) f();
    };
    return std::unique_ptr<void, decltype(x)>(
        (void*)(intptr_t)(std::uncaught_exceptions() + 1), std::move(x));
}

auto rollback = on_throw([&]{ do_rollback(); });

In both cases, disarming is easily done by calling .release().


Using the common extension __COUNTER__ and some preprocessor-tricks as ssuggested by user2176127 in a comment below makes it even more convenient:

#define UNIQUE_ID_1(x) _Unique_Id_##x
#define UNIQUE_ID_0(x) UNIQUE_ID_1(x)
#define UNIQUE_ID UNIQUE_ID_0(__COUNTER__)

#define FINALLY(...) auto UNIQUE_ID = finally([&]{ __VA_ARGS__; });
#define ROLLBACK(...) auto UNIQUE_ID = finally([&]{ __VA_ARGS__; });

FINALLY(final_action());
ROLLBACK(do_rollback());
Deduplicator
  • 8,591
  • 5
  • 31
  • 50
  • The use of `std::unique_ptr` in this fashion is smart and hackish, which fits into the theme of [HHXX](https://github.com/Lingxi-Li/Happy_Hacking_CXX#happy-hacking-cxx-library-hhxx) :-) But the methods provided by `std::unique_ptr` are way too irrelevant and unintuitive. Users can even call `operator*()` to get undefined behavior. Rearm would need to call `reset()`. Also, it seems that you cannot pass `void` to `std::unique_ptr`. – Lingxi Jan 29 '16 at 13:54
  • Well, if you don't like to use release for disarming, or want a method for rearming, yes, you simply have to write your own class. Might lead to a nicer debug-experience too ;-) But if the second template-argument is not defaulted, why should the first not be void...? [G++ at least has no trouble with it](https://goo.gl/iXsgnz), and I cannot find anything on [cppreference.com](http://en.cppreference.com/w/cpp/memory/unique_ptr) which might cause concerns. – Deduplicator Jan 29 '16 at 14:15
  • Thanks for the helpful discussion. Just updated the [code](https://github.com/Lingxi-Li/Happy_Hacking_CXX/blob/master/hhxx/scope_guard.hpp). I guess everybody is happy now :-) – Lingxi Jan 29 '16 at 14:56
  • I just posted a S.O. question about the `std::unique_ptr` issue. Guess you would be interested :-) See [here](http://stackoverflow.com/q/35098466/1348273). – Lingxi Jan 30 '16 at 05:45
  • "For simplicity I used C++14 automatic return-type-deduction" How would you do this with only C+11? – Baruch Nov 09 '17 at 12:31
  • 1
    This sounds like a good use case for `[[nodiscard]]`. I'm also using this via a little macro for extra convenience `#define FINALLY(x) auto _Finally_##__COUNTER__ = finally(x);` – user2176127 Oct 21 '18 at 14:05
  • Forgot to fully expand the macro, here is fixed pasta `#define FINALLY_EXPAND1(x, y) auto _Finally_##y = finally(x) #define FINALLY_EXPAND0(x, y) FINALLY_EXPAND1(x, y) #define FINALLY(x) FINALLY_EXPAND0(x, __COUNTER__)` – user2176127 Oct 21 '18 at 15:22