In the following example, a mutex is used to protect a file:
#include <fstream>
#include <memory>
#include <mutex>
std::mutex m;
int main() {
std::unique_ptr<std::lock_guard<std::mutex>> ptr_1 = std::make_unique<std::lock_guard<std::mutex>>(m);
std::fstream file_1("file_name.txt");
std::unique_ptr<std::lock_guard<std::mutex>> ptr_2{std::move(ptr_1)};
}
When the destructors are called, the mutex will be unlocked before the file is closed. This can cause a race condition. It seems to me that there must be some guideline about move-operations, destructors, or ownership that I don't know about. What design guideline has been broken?
There are two guidelines/best practices that I see are violated here.
The first is enshrined in the C++ Core Guidelines as R.5:
Prefer scoped objects, don’t heap-allocate unnecessarily
Using heap allocation removes the structure afforded by the lexical scoping rules in C++. It is effectively an assertion that the programmer will manage lifetimes.
If you do not heap-allocate, the code doesn't compile:
std::lock_guard<std::mutex> lg_1{m};
std::fstream file_1("file_name.txt");
std::lock_guard<std::mutex> lg_2{std::move(lg_1)};
The second is CP.50:
Define a mutex together with the data it guards
The spirit of that rule is to encapsulate the synchronization. Put the data and lock together, and expose an API that is more difficult to misuse. Many designs would still have an RAII guard, because that is a flexible option, so you still have to put that on the stack. A guard type isn't strictly necessary though.