isocpp
Feature Request: gsl::finally.ignore()
C++ exceptions are amazing, but they introduce an issue with how to handle state. The commit / rollback pattern is amazing at handling this:
http://stackoverflow.com/questions/10983791/c-transaction-like-pattern-for-all-or-nothing-work
Basically, it allows you to do the following:
auto cor1 = CommitOrRollback([]{
undo action1
});
action1();
auto cor2 = CommitOrRollback([]{
undo action2
});
action2();
cor1.commit();
cor2.commit();
gsl::finally is exactly the same thing, but is missing the "commit" function. Since the "finally" operation is sort of backwards, I propose calling it "ignore" and can be implemented doing the following (tested and already working in our repo).
void ignore() noexcept { invoke_ = false; }
This added API would not affect how it is currently used, but would add the ability to prevent the action from executing based on a condition, which means it would be able to be used for rolling back state in the event of an exception, something that IMO should be integral to the C++ stack.
Also on the GSL tracker: https://github.com/Microsoft/GSL/issues/335#issuecomment-241073023
Personally I'd rather call it dismiss, but I think the feature would indeed be useful. It might be interesting to see if this has any significant influence on the codegen when it is not used.
But... if there are parts of the code that require you to call ignore() have you not sort of just re-introduced the kind of problem that finally was created to solve? What if you forget to call ignore()?
@galik What if you forget to write the finally in the first place? All we are doing here is adding an additional capability to finally so that it can be used for the commit / rollback pattern that addresses a pretty significant issue with C++ exceptions.
finally() does exactly what it is intended to do: install an operation to be executed at the end of scope. It is less disciplined than an object with a more specific destructor, but it can be used to implement a commit. A major advantage of destructors and RAII is that the remove the opportunity to forget to clean up a constructed object. Having explicit close, dispose, release, commit, unlock, etc. actions is an opportunity to forget, to get bugs.
What about adding another class to the GSL that is a more traditional "commit or rollback"? Rolling back state changes in the event of an exception is a HUGE issue with C++ exceptions and using RAII is one of the best ways to deal with this issue (the alternative is to try, catch, rethrow, which is far more likely to introduce bugs).
A great example of this is the fact that we state with the STL to only use mutex lockers. One of the reasons for this is in the event of an exception, the locker will unlock the mutex preventing deadlock. Nothing exists in the STL or GSL to provide a means to rollback state as state changes are being made to a class.
Shouldn't we encase the state transaction in a one-purpose dedicated class and use it as RAII, instead of a generic lambda holder ? Lambdas always feel a bit spaghettic to me.
I disagree. Lambda's really simplify what is needed to handle rolling back state. I submit one of my better examples (will be presenting this example at CppCon):
https://github.com/Bareflank/hypervisor/blob/master/bfvmm/src/vmxon/src/vmxon_intel_x64.cpp#L38
We are currently using C++11/14 to implement a hypervisor, and when your turning on VMX (Intel's VT-x), you have to go through a set of steps, each one changing the state of the CPU. If a failure occurs, which can absolutely happen, the gsl::finally lambda functions reverse the operations that took place, putting the CPU back into the original state it was in. At the very end when everything worked, we "commit" our changes which prevents the gsl::finally calls from rolling back. We then use an extensive set of unit tests and Coveralls to verify that all of this works as expected.
Obviously this is using my proposed .ignore extension, which we could change to a different class in the future if that's what you decide. We used to use our own commit and rollback class, but gsl::finally literally was the exact same code, minus the one function I am proposing.
Let me see if I understand: You use finally to roll back, rather than commit, thus you need to make sure it doesn't happen for successful completion of the function; right? I presume that the reason for that is that you want rollback on exceptions. Shouldn't such an operation be called undo (or rollback) rather than finally? It seems different.
Yes. We are using finally simply because the internal implementation is exactly the same thing we had which is in the link above. It was called CommitOrRollback. Since the code is the same we could either add 'dismiss' to 'finally' and typedef it to give it a better name or use inheritance to add 'dismiss', 'commit' or whatever to a subclass with a better name.
Would an ideal feature be one that fired when an exception was thrown, but not for normal return (through a return statement or by falling off the end)?
Reminds me of Alexandrescu's talk at CppCon last year on Declarative Control Flow... he defined three macros with statements to be executed when leaving a scope: only on fail(rollback state on exception), only on success (normal return), or both (a la destructor):
https://github.com/CppCon/CppCon2015/tree/master/Presentations/Declarative%20Control%20Flow On Aug 27, 2016 12:45 PM, "Bjarne Stroustrup" [email protected] wrote:
Would an ideal feature be one that fired when an exception was thrown, but not for normal return (through a return statement or by falling off the end)?
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/isocpp/CppCoreGuidelines/issues/688#issuecomment-242927655, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGPBbSGjwq_yFj79NmT5STXsV7VnftSks5qkGmUgaJpZM4JoqrL .
In this case, yes, only executing on an exception would work and its the main reason we use this pattern. That being said, is it possible to implement such a feature? Also, it would require the use of exceptions which might be too specific. What we are really doing here is removing the typical 'goto failure' and clean up code that you would typically see in kernel programming. I could see some people interested in this feature who simply want to return a failure code without having to use a goto for cleanup.
Yes, these ideas are as old as the mountains; the issue is if, how, and what to support directly in the guidelines,
IMO not only should it be supported by the guidelines, but there should be an example given to demonstrate the issue, and how this pattern can be used to solve it. When we were deciding to use exceptions, the two issues that most articles talked about with respect to C++ exceptions were complexity, and state corruption. I don't buy the complexity argument as they are not that complex, and you have to deal with errors somehow, but the state corruption problem is an issue, and RAII is a clean way to address it.
We could use an example like:
class foo
{
public:
foo() = default;
~foo() = default;
bool bar(int val, const std::string &str)
{
if (!value) {
return false;
}
m_a = val;
m_b = std::stoi(str, nullptr);
if (!m_b) {
return false;
}
m_sum = m_a + m_b;
return true;
}
private:
int m_a;
int m_b;
int m_sum;
};
bar in this case returns false if any input is 0, otherwise it adds the two numbers and stores the numbers and the result. The "rule" in this case would be... never change the internal state of a class, without ensuring state consistency (or something along those lines). In this example there are two potential issues here:
- The code should return false if any input is 0. The conversion of the str to an int must take place before checking if it is 0. If it is 0, m_a and m_b are modified, m_sum is not and thus the function will exit leaving the internal state of the class corrupt. Granted... you might be able to reorder some of these operations in this example to prevent this, but that's not always the case.
- During the process of converting the str to an int, and exception could be thrown that would leave m_a modified, but not m_b and m_sum, meaning the state is corrupt again.
A simple solution, and more complete function would be:
class foo
{
public:
foo() = default;
~foo() = default;
bool bar(int val, const std::string &str)
{
auto old_a = m_a;
auto old_b = m_b;
if (!value) {
return false;
}
auto cor1 = commit_or_rollback([&]{
m_a = old_a;
});
m_a = val;
auto cor1 = commit_or_rollback([&]{
m_b = old_b;
});
m_b = std::stoi(str, nullptr);
if (!m_b) {
return false;
}
m_sum = m_a + m_b;
cor1.commit();
cor2.commit();
return true;
}
private:
int m_a;
int m_b;
int m_sum;
};
This updated function ensures that regardless of how the function exits, state is always left in a consistent, transactional form. If the function succeeds, all member variables are updated as expected. If any failure occurs, including an exception, non of the memory variables are effected (i.e. the internal state is not left corrupt on failure).
Here I used the commit_or_rollback name. This could be implemented in the GSL by simply subclassing gsl::final_act and adding a commit function that tells the destructor to not execute the final act function.
Hi @rianquinn
I think you have a very interesting but difficult problem here.
I may have misunderstood something or gotten things wrong so forgive me if I am coming at this through some lack of understanding. Feel free to ignore these comments if they don't apply or correct me if I have mistaken the situation.
When you are using rollback calls to deal with exceptions then you run the risk of accidentally tripping an exception in one of those calls which would lead to program termination. I think this is where the complexity comes in. It can easily become difficult to follow the code.
Looking at example you provided here: https://github.com/Bareflank/hypervisor/blob/master/bfvmm/src/vmxon/src/vmxon_intel_x64.cpp#L38
One of the rollback functions (execute_vmxoff()) can throw an exception. If that happened then the program would terminate immediately.
Also your function create_vmxon_region() uses its own finally to call release_vmxon_region() after you already provided one before calling create_vmxon_region() so that in the event of an exception inside create_vmxon_region() the release_vmxon_region() function would be called twice.
Add to that the possibility of forgetting to call commit() may cause hard to find and unexpected silent bugs.
I just seems to me there is room for errors with this approach. Especially when, as part of your rollback you are calling other functions during an exception.
With the basic example you provided in this thread, I have dealt with that situation in the past by creating a new state object for the class, making all the changes to the new state object and if all changes succeed swapping the new state object with the current state object for the class.
class Foo
{
private:
state_type state; // transactional state
int k; // non-transactional state
public:
void transaction_func() // may throw
{
state_type state;
// try to change part of state (may throw)
// try to change another part of state (may throw)
// commit the new state
swap(this->state, state); // can not throw! (strong exception guarantee)
}
}
But that may not work for your hypervisor. Especially if, for example, the state changes are actually to a hardware device. In that case what you are really after is the ability to unconfigure what was previously configured if not all of the configuration succeeds. But a difficulty arises when one of the unconfiguring actions also fails! How do you deal with that?
Unless the device itself supports commit/rollback it seems to me that the least complex and most robust coarse is to accept, in the face of an exception, that the device is badly-configured and needs to be reset to some working default. Also the software, would need to throw an exception that is caught in such a place that is designed to move forward from the reset condition. Trying to unconfigure your way out of it during an exception feels problematic to me.
As an example:
void device::transaction_func()
{
try
{
if(!set_mode("A"))
throw;
if(!set_param("X", 4))
throw;
if(!set_param("Y", 0))
throw;
}
catch(...)
{
reset(); // cause device to reset to a safe/known state
// go somewhere you can explain to the user and get
// them to try again/try another action/report a bug
throw device_reset_exception();
}
}
That, of course, does not solve your problem it basically ignores your problem in favor of "giving up" and staring from scratch. I just feel that might be the safest approach to these situations. The problem of "what to do" when encountering an error while undoing what you previously did seems fairly intractable to me.
So that's what I brought up before... this really makes the code worse because every single function would need a try, cach, rethrow like you have above, which is IMO.... terrible, and causes other issues. First you assume that such a pattern somehow reduces complexity which I would argue is not the case, and secondly, the above pattern does not scale with complexity like goto, which I will explain below.
I would add that the rollback is executed in the context of a destructor, which is labeled noexcept by default, which means that if the rollback code causes an exception, std::terminate is called. There are two different approaches to this... you can either make sure that the rollback code is also labeled noexcept, or you can put a try catch in the destructor, or simply let std::terminate be called. This should be left to the user to figure out.
In our case, our "release" code is designed to be executed more than once, and we unit test that case. Currently our code has a try catch in it, but that will likely be changed to be labeled noexcept in the future. Also, since we call gsl::finally.ignore(), it's only executed once in most cases (not all), but it ensures that state is handled properly in our case and we test for each scenario. It just depends on what your doing. This is why unit testing is so important for any code / project.
I think it's a mistake to not provide simple means for handling state rollback in C++. Also.. in your example, the try catch does not handle the rollback properly (its would have a bug in my above example). To do this with try, catch, rethrow, you would need:
class foo
{
public:
foo() = default;
~foo() = default;
bool bar(int val, const std::string &str)
{
auto old_a = m_a;
auto old_b = m_b;
if (!value) {
return false;
}
try {
m_a = val;
} catch (...) {
throw;
}
try {
m_b = std::stoi(str, nullptr);
} catch (...) {
try {
m_a = old_a; // Restore a, just as complex
} catch (...) {
rethrow;
}
}
if (!m_b) {
try {
m_a = old_a; // <- no try catch here... simply need to remember to restore state here
} catch (...) {
throw;
}
try {
m_b = old_b; // <- no try catch here... simply need to remember to restore state here
} catch (...) {
throw;
}
return false;
}
m_sum = m_a + m_b;
return true;
}
private:
int m_a; // <- assume for this example that operator=() could throw
int m_b; // <- assume for this example that operator=() could throw
int m_sum;
};
As this example grows in complexity... so does the restore logic. This is why kernel programmers use goto, as they can create one block of rollback code and jump to the location in the rollback code thats needed. The commit_or_rollback pattern provides this same level of, "add rollback for each step" logic that a goto provides, without the need for goto, while also supporting exceptions. That's why if you look at our hypervisor code example above, each commit_or_rollback is created just prior to the next step. If the commit_or_rollback is not created, it is not executed and that step is not rolled back, just like a goto block.
@galik
One thing I might add is that if it's called something other than gsl::finally, the analysis tool (e.g. Clang Tidy) could easily detect the creation of a commit_or_rollback object without a call to commit which could be stated as a rule being "invalid". In other words you could state in a rule, always call commit when using commit_or_rollback which would remove the worry of "what if you forget to call commit". The tool would tell you that your not calling it, so either call commit or change to gsl::finally. Later on down the road.... this could be added as a first class citizen to the language if needed. Something like:
void foo()
{
[[rollback]] {
// fix state here
}
}
In which case, if the end of the function is reached, the compiler could prevent the rollback from occurring, and you could even support a [[success]] if you want to manually commit. Just a thought.
I dug up the original articles that lead us to this implementation. Also note that some of the above arguments (like the rollback code throwing) are already an issue with gsl::finally. All I am proposing is adding an API to an existing capability that is already supported by the GSL so that it can be used for transactional operations. The point of these links is to not only provide supporting evidence, but to also show that I'm not the only one that has run into this issue.
http://www.drdobbs.com/cpp/generic-change-the-way-you-write-excepti/184403758
https://channel9.msdn.com/Shows/Going+Deep/C-and-Beyond-2012-Andrei-Alexandrescu-Systematic-Error-Handling-in-C
http://stackoverflow.com/questions/10983791/c-transaction-like-pattern-for-all-or-nothing-work
A proposal is working its way through the committee: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0052r1.pdf and http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0052r0.pdf .
I don't like the names "scope_" is IMO verbose and misleading. The factory functions are unnecessary with template argument deduction in C++17, and the classes seems bloated/overdesigned with deleters, reset(), reset(), etc.
Opinions?
I agree with your complaints on the implementation, but the idea of being able to get a final_act to only execute on success, failure or always is exactly what we are looking for. I also really like this because it would remove the need for the commit function, and it's explicit as to what you want (i.e. it's self documenting). I also don't think it would take a lot to implement this in final_act as it is today, meaning this is something we could start using now, instead of waiting for the STL to adopt it.
My only additional concern is this would once again require the use of exceptions, and there might be people interested using something like this with applications that use "-fno-exceptions". That being said... they could always just use the code we are currently using in their own tree. IMO this solves an issue that C++ exceptions have had for sometime, so I don't see anything wrong with the solution only applying to exceptions.
Also... I didn't know this existed std::uncaught_exceptions(). Learned something new 😄 .
One issue I have with the implementation that is proposed is the use of std::unique_ptr. If std::bad_alloc occurs, this could pose an issue as the cleanup code might not execute. IMO, gsl::final_act is better as it doesn't require a malloc.
Is this implementation more inline with what your after? I tested it and it works well. One thing I like about this is, it could be used with "-fno-exception", but includes the controversial dismiss. This is modeled after gsl::final_act with the dismiss function and setting the destructor as virtual
#include <utility>
#include <functional>
class execute_on_exit
{
public:
explicit execute_on_exit(std::function<void()> &&func) noexcept :
m_executed(false),
m_func(std::move(func))
{
}
execute_on_exit(execute_on_exit &&other) noexcept :
m_executed(other.m_executed),
m_func(std::move(other.m_func))
{
other.m_executed = true;
}
execute_on_exit(const commit_or_rollback &) = delete;
execute_on_exit& operator=(const commit_or_rollback &) = delete;
virtual ~execute_on_exit()
{
if (!m_executed) m_func();
}
void dismiss() noexcept
{ m_executed = true; }
private:
bool m_executed;
std::function<void()> m_func;
};
class execute_on_success : public execute_on_exit
{
public:
explicit execute_on_success(std::function<void()> &&func) noexcept :
execute_on_exit(std::move(func))
{ }
execute_on_success(execute_on_success &&other) noexcept :
execute_on_exit(std::move(other))
{ }
execute_on_success(const execute_on_success &) = delete;
execute_on_success& operator=(const execute_on_success &) = delete;
~execute_on_success() override
{
if (std::uncaught_exception())
this->dismiss();
}
};
class execute_on_failure : public execute_on_exit
{
public:
explicit execute_on_failure(std::function<void()> &&func) noexcept :
execute_on_exit(std::move(func))
{ }
execute_on_failure(execute_on_failure &&other) noexcept :
execute_on_exit(std::move(other))
{ }
execute_on_failure(const execute_on_failure &) = delete;
execute_on_failure& operator=(const execute_on_failure &) = delete;
~execute_on_failure() override
{
if (!std::uncaught_exception())
this->dismiss();
}
};
Note that overtime, GSL implementations will be removed in favor of the standard library implementations (when the corresponding specs are adopted).
@gdr-at-ms agreed, but as @BjarneStroustrup stated, this spec is still under review, and this might be a good opportunity to propose a cleaner approach that is more inline with what the GSL currently already has.
I have found the discussion on this issue very interesting. And I have a couple of comments.
- @rianquinn in your more recent example you use std::function<void()> but you also say you want to avoid malloc's. There are cases where std::function will malloc (certainly in Microsoft implementation). I assume this why in the standard, they use a templates parameters for the functions, they much more efficient and won't result in allocations.
- The controversial dismiss method :) I think that std::uncaught_exception is the most important thing (for new code). But I can see uses for these mechanism in my codebase, which is still all COM :( even as we remove the MIDL files and move to more modern C++ we still return HRESULTs from all methods. Our ultimate goal would be to switch to exceptions, but the code base is heavily used and over 15 years old, we haven't got a solution to this refactor. The dismiss methods allow the scope helpers to at least be usable in these cases.
- It would be good to see the draft in the standard to updated, if others agree.
- @gdr-at-ms the GSL currently supports VS2013, is there thought to how long this will be true? I could see replacing GSL features with standard library will break support for older compilers. We tend to update VS version every 3-4 years we are stuck on VS2013 for a while, we just missed the release of VS2015 :(
VS 2013 does not support constexpr and noexcept, not to the extent that we would like to provide much better experience. We don't expect that support to last for a very long time -- but @neilmacintosh is the definite source for how long that is going to last.
@KrisTC we're off-topic here, but just quickly on the topic of VS2013 support: I plan to retire VS 2013 support to a branch when the next version of Visual Studio is released. In other words, I intend to support only the current and immediately previous release of MSVC at any point in time. If you want to follow up on the topic, it's probably best to do so over at the GSL repo ;-)
@KrisTC I didn't know about std::functional using malloc so yeah, I'll post a modified version once I have a chance to test it that uses templates. std::functional has the advantage that if an improper function is provided, the compiler can provide a sane error, where as templates tend to provide insane errors when your using them incorrectly. As for dismiss, I am still a huge fan of it, just depends on what others say. I started porting our code over to use this implementation instead to give it a try in an actual project and see how it goes and we do have a couple places were it is still needed (mainly in the areas where we write extern "C" functions that cannot throw.
Ok, here is a new version of this that uses gsl::final_act and templates as suggested by @KrisTC. The only mods to gsl::final_act are the addition of the dismiss function, and the destructor is labeled virtual. This also provides the convenience functions named on_success and on_failure which mimic gsl::finally.
Thoughts?
template <class F>
class final_act
{
public:
explicit final_act(F f) noexcept : f_(std::move(f)), invoke_(true) {}
final_act(final_act&& other) noexcept : f_(std::move(other.f_)), invoke_(other.invoke_)
{
other.invoke_ = false;
}
final_act(const final_act&) = delete;
final_act& operator=(const final_act&) = delete;
virtual ~final_act() noexcept
{
if (invoke_) f_();
}
void dismiss() noexcept
{
invoke_ = false;
}
private:
F f_;
bool invoke_;
};
template <class F>
class final_act_success : public final_act<F>
{
public:
explicit final_act_success(F &&func) noexcept :
final_act<F>(std::move(func))
{ }
final_act_success(final_act_success &&other) noexcept :
final_act<F>(std::move(other))
{ }
final_act_success(const final_act_success &) = delete;
final_act_success& operator=(const final_act_success &) = delete;
~final_act_success() override
{
if (std::uncaught_exception())
this->dismiss();
}
};
template<class F>
class final_act_failure : public final_act<F>
{
public:
explicit final_act_failure(F &&func) noexcept :
final_act<F>(std::move(func))
{ }
final_act_failure(final_act_failure &&other) noexcept :
final_act<F>(std::move(other))
{ }
final_act_failure(const final_act_failure &) = delete;
final_act_failure& operator=(const final_act_failure &) = delete;
~final_act_failure() override
{
if (!std::uncaught_exception())
this->dismiss();
}
};
// finally() - convenience function to generate a final_act
template <class F>
inline final_act<F> finally(const F& f) noexcept
{
return final_act<F>(f);
}
template <class F>
inline final_act<F> finally(F&& f) noexcept
{
return final_act<F>(std::forward<F>(f));
}
// on_success() - convenience function to generate a final_act_success
template <class F>
inline final_act_success<F> on_success(const F& f) noexcept
{
return final_act_success<F>(f);
}
template <class F>
inline final_act_success<F> on_success(F&& f) noexcept
{
return final_act_success<F>(std::forward<F>(f));
}
// on_failure() - convenience function to generate a final_act_failure
template <class F>
inline final_act_failure<F> on_failure(const F& f) noexcept
{
return final_act_failure<F>(f);
}
template <class F>
inline final_act_failure<F> on_failure(F&& f) noexcept
{
return final_act_failure<F>(std::forward<F>(f));
}
Thanks. I have a hard time seeing the need for the dismiss function. Do you consider it an implementation detail (in which case it should not be public) or is there an important use case?
The use of a virtual (destructor) is unfortunate because it complicated layout and could encourage people to build their own derived classes. Do you see a need for such derivation?
I see the three functions: on_success(), on_failure(), and finally() as fundamental (whatever we call them) and more and more complicated variants would do little except dilute the idea.
I wonder, if there is no manual way to indicate to the scope guard that we failed or succeeded and we are soley relying on the presence of exceptions for that, maybe on_failure should be renamed to on_except?
@BjarneStroustrup the best example I have for the need for dismiss are functions that cannot use exceptions. @KrisTC brought up a great example where he would like to use on_success(), on_failure() but cannot because he is dealing with legacy code that would require manually asserting success / failure (i.e. no exceptions). I could implement on_success / on_failure without derivation which would remove the need for dismiss and the virtual destructor (would add duplication to the library but that's probably fine). I would prefer to leave dismiss in the revised code to enable it's use in legacy applications and code that doesn't use exceptions.
For C++17, we will not need the factory functions (and the ability to move) because we will have template argument deduction from constructors arguments.
I don't see the connection between dismiss() and "no exceptions"; please explain. I see on_return(), on_error(), and finally() as means of using exceptions in a principled way, and would consider supporting non-exception code as secondary or separate. I see try/catch as the assembly code for exceptions (and did in fact not introduce exceptions into C++ until I had figured out RAII as the key way to use them - the functions we are discussing here fits into that view).
@BjarneStroustrup For example, suppose I am working on a library written in C++, that exposes C-only functions (we do this a lot in our hypervisor, and @KrisTC seemed to have a similar issue with legacy code). In our case, libc++ needs a "C" library to perform some of it's work. Since we have to bootstrap this entire environment (with a hypervisor there is no OS to rely on), we have two choices, write C code (uhg) or write C++ code with extern "C". We choose to use the later.
In these cases, you still have "exception" like functionality, your just doing it manually. In C, a lot of the time you "bubble up" an error code by dedicating the return of a function always be an error code (you see this a lot in Windows APIs). An example might be:
NTSTATUS foo(void)
{
NTSTATUS status;
status = WindowsAPI_1()
if (! NT_SUCCESS(status)
goto failure;
status = WindowsAPI_1()
if (! NT_SUCCESS(status)
goto failure;
<do something>
failure:
<cleanup>
return status;
}
This code is a lot cleaner with RAII. gsl::finally is exactly what you need, minus the fact that you need a manual way to state "all is good".
NTSTATUS foo(void)
{
NTSTATUS status;
auto fa1 = gsl::finally([&] {
<cleanup>
});
status = WindowsAPI_1()
if (! NT_SUCCESS(status)
return status;
auto fa2 = gsl::finally([&] {
<cleanup>
});
status = WindowsAPI_1()
if (! NT_SUCCESS(status)
return status;
<do something>
fa1.dismiss();
fa2.dismiss();
}
Now I'm not saying there aren't other ways to do this, I'm just saying adding dismiss allows a simple means for finally (or RAII really) to be used in more cases to modernize code and remove potential bugs. on_return and on_error would still be used (so that they are self documenting), but since an exception is not being thrown, you have to manually dismiss the execution of the RAII function.
It's kind of like a std::lock_guard in that, you can always call mutex::unlock if needed, even if your using the std::lock_guard. There are times when it needs to be done.
I know this is a pretty awful piece of code by modern standards. This isn't a real example, but we have lots of old code like this. Methods that return HRESULTS and uses Win32 API's. Using finally can improve the code readability and safety.
struct FileRecordReader {
FileRecordReader() : m_file(nullptr);
~FileRecordReader() { if (m_file) ::Close(m_file); }
struct HEADER;
struct RECORD;
HRESULT OpenFile() {
FILE *f = nullptr;
if (!::Open("somefile.txt", f)) return E_FAIL;
auto clean1 = gsl::finally([&] { if (f) ::Close(f); });
HEADER header;
if (!ReadHeader(f, &header)) return E_FAIL;
if (!header.type != "the type we wanted") return E_FAIL;
if (m_file) ::Close(m_file);
// Ok it's all good
m_file = f;
clean1.dismiss();
return S_OK;
}
HRESULT ReaderHeader(FILE* f, HEADER* h) {
if (!::Read(f, h, sizeof(r))) {
return E_FAIL;
}
return S_OK;
}
HRESULT ReadRecord(RECORD* r) {
if (m_file == nullptr) return E_FAIL;
if (!::Read(m_file, r, sizeof(r))) {
return E_FAIL;
}
return S_OK;
}
FILE* m_file;
};
See the MSDN example: https://msdn.microsoft.com/en-us/library/windows/desktop/aa364640(v=vs.85).aspx
[Edit] Annoyingly every time I come up with an example like this I come up with a solution that is cleaner and doesn't require dismiss() method :/
Change from:
if (m_file) ::Close(m_file);
// Ok it's all good
m_file = f;
clean1.dismiss();
Change to:
std::swap(f, m_file);
@KrisTC I think that's why this topic has not been brought up before because in a lot of cases people are not talking to external logic. When dealing with hardware, a class can encapsulate an external device, but std::swap is no longer useful. Now that C++ is getting more traction in low level work (a good example is the IncludeOS unikernel), this type of problem will become more and more of an issue (i.e. std::swap cannot be used to get out of the problem).
Usually when I see a new feature which cannot entirely fit into an existing idea, I try to think of it entirely as something "new" and see where it will lead me.
In this case what about dismissable_finally? (or finally<dismissable>)
This conveys the intention better than just finally - It will be clear that this action can be dismissed.
You don't pay for what you don't use - finally will have no extra bool, no extra checks, only on request.
This gives the ability to use the classes in a "no-exception" environment.
In finally<dismissable<F>>, dismissable could use a lightweight form of std::stop_token to make its call operator a noop. Seems like a good separation of concerns and composition.
