1

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?

Mark Wallace
  • 528
  • 2
  • 12
  • 1
    To me, it's a bit smelly that you need a pointer to a mutex. Usually, a mutex is in an object which has only very few instances and thus you don't move it around, but that's just a hint. – lorro Jul 07 '22 at 21:36
  • 2
    You have two unrelated mutexes, it doesn't matter in what order they are destroyed. This code is perfectly normal. – Osyotr Jul 07 '22 at 21:36
  • @Osyotr I'm currently working on an example that is (Step 1) ```lock_guard``` a mutex, (Step 2) open a file (protected by the mutex), (Step 3) call ```lock_guard``` destructor, (Step 4) close the file. You may find that example more convincing. – Mark Wallace Jul 07 '22 at 21:46
  • I don't think the programmer would be tempted to swap this `lock_guard` thing and a file because they would know the mutex must be locked at all times while the file is open. Also those two things would probably have different classes, so the compiler wouldn't allow them to be swapped. – David Grayson Jul 07 '22 at 21:48
  • @DavidGrayson construct ```lock_1``` (in a ```unique_ptr```), construct ```file_guard_1```, construct ```file_guard_2```, move ```file_guard_1``` to ```file_guard_2```, move ```lock_1``` to new ```unique_ptr``` (lock_2). This will unlock the mutex before closing the file. – Mark Wallace Jul 07 '22 at 21:51
  • In this latest example, the presence of a bug depends on when exactly this new unique_ptr gets destroyed, which could be at any time if its lifetime is managed with `new` and `delete`. As I've said, the programmer should actually know what they are doing and think about lifetimes before randomly moving objects that hold important items like mutexes, file handles, and sockets. And if you can't trust them, make a wrapper class that takes care of all the details and manages all the lifetimes for them. I think you're sort of discovering why people like garbage collectors or borrow checkers. – David Grayson Jul 07 '22 at 21:55
  • The design rule that's been broken is **When handling synchronization objects, think about synchronization**. Yes, that implies pretty strong restrictions on the ability to encapsulate synchronization concerns. – Ben Voigt Jul 07 '22 at 22:11
  • @MarkWallace please stop editing your post in a way that **invalidates existing answers and comments**. You completely re-wrote your post from scratch to something different from its original form. I rolled back that edit and appended your new example, then you restored your edits. I have rolled it back again. Any further edits should not alter the original text, please. – Remy Lebeau Jul 07 '22 at 22:11
  • @RemyLebeau The question has always been the title. The example I gave before was just an example, which I made clear in the question. It was an unclear and (according to comments) invalid example. No one should have to read long sections of code that are made redundant by newer, better examples. – Mark Wallace Jul 07 '22 at 22:13
  • 1
    @RemyLebeau: I think I agree with Mark here, while the rewrite has a different form, it is about the same question. And since it still involves a mutex, the answers still make sense. Invalidating existing comments is NOT a problem. – Ben Voigt Jul 07 '22 at 22:15
  • @BenVoigt Whatever – Remy Lebeau Jul 07 '22 at 22:16
  • @BenVoigt "When handling synchronization objects, think about synchronization." Part of my concern is that I can't convince myself that the "problem" is isolated to synchronization. However, I can't come up with an example that isn't based on thread synchronization. – Mark Wallace Jul 07 '22 at 22:28
  • In the (current?) code shown, there is no mutex. There is a smart pointer that holds null, and then that null is later moved to the other smart pointer. I suspect the answer to the title question is: C++ is not a nanny language, it gives you enough rope to shoot yourself in the foot. – Eljay Jul 07 '22 at 22:42
  • @Eljay thanks, fixed. Responding to "C++ is not a nanny language": There are nice rules of thumb that can help us write error-free code. Many of these rules are found in the [core guidelines](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines). Part of the question here is "is there a guideline that I have broken?" The guideline "don't shoot yourself in the foot" isn't useful for anyone. – Mark Wallace Jul 07 '22 at 22:48
  • 2
    `std::lock_guard` guards a *scope*. Which scope does your lock guard? – n. m. could be an AI Jul 07 '22 at 22:57
  • You are violating R.1, and going to some effort to do so due to circumventing the non-copyable non-moveable with a unique_ptr. C++ won't prevent you from self-harm. – Eljay Jul 07 '22 at 23:42
  • @Eljay this code does not violate anything in R.1. In fact, it was written to follow R.1. – Mark Wallace Jul 08 '22 at 00:12
  • Then don't use unique_ptr. Because by using unique_ptr, it's thwarting R.1 and RAII with the improper order of destruction. Or wrap the `file_1` in `{...}`. – Eljay Jul 08 '22 at 00:28
  • I think the rule that you are missing is that automatic variable are destructed in their scope in the opposite of their order of creation. `int a; int b; int c;` get destructed c, b, a. Or in your example: ptr_2, file_1, ptr_1. – Eljay Jul 08 '22 at 02:15

3 Answers3

1

In general, C++ programmers should be aware that different C++ objects have different lifetimes and get destroyed at different points in time. If you have an object that holds some special items (e.g. mutexes, file handles, sockets) and you have rules about the required lifetime of those items, then it's up to the programmer to know those rules and consider them before they move items from one object to another.

I wouldn't say any design rule was broken in your example, I would just say the programmer introduced a bug when they added the move operation without thinking about the obvious implications of that.

David Grayson
  • 84,103
  • 24
  • 152
  • 189
  • But changing the order of unlock breaks "you only lock a child if you currently are locking its parent" – Ben Voigt Jul 07 '22 at 22:07
  • OK, thanks, you're right, so I'll just remove the stuff about mutexes. It would be possible to make my rules a little more complicated to accommodate unlocking reordering but I don't want to get into that. – David Grayson Jul 07 '22 at 23:06
1

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.

Jeff Garrett
  • 5,863
  • 1
  • 13
  • 12
  • I do not believe this code violates R.5. Sure, the ```lock_guard``` was unnecessarily allocated on the heap. However, the **user** did not allocate on the heap. The user just constructed a ```unique_ptr``` on the stack. The heap allocation was done in library code the user may not have seen. And, obviously, the library code did not violate R.5. (The assumption here is that the heap allocation could be done by combining extremely complicated non-standard library code. Perhaps the library code defines a type ```Wrapper``` that contains a ```unique_ptr``` and a move-operator.) – Mark Wallace Jul 08 '22 at 18:57
  • I'm looking carefully at CP.50 and ```synchronized_value```. I believe you are correct that the OP code violates this guideline and ```synchronized_value``` fixes the problem. Ideally, an answer to the OP would address why reordering destructors shouldn't cause problems in non-synchronization code (assuming users follow guidelines), but this solves the synchronization case. – Mark Wallace Jul 08 '22 at 19:05
  • I think the R.5 part addresses the destructors. There are two RAII objects and they are destroyed in the expected order (reverse order, lexically scoped). Moving ownership of a resource from one to the other would be safe (by api design), except we put them on the heap and tell the compiler we're in charge of keeping them out of trouble. Remove the unnecessary heap from the question and it's not a problem. – Jeff Garrett Jul 08 '22 at 20:18
  • Regarding the assumption on the allocation being behind a library interface... That's wasn't clear from the question, but it doesn't change things too much. The problem is then in that library code. They could have made the lock_guard a member, but if they put it on the heap. They then have a choice in how to implement move, but if they want it to act like the inner type, you would disable move. This is more hypothetical without an example. – Jeff Garrett Jul 08 '22 at 20:28
  • I stand by my assessment that the OP code does not violate R.5 because (1) the user does not make heap allocations and (2) the STL ```std::unique_ptr``` does not violate R.5. I challenge you: create a destructor-order bug example while following CP.50 **and ownership semantics** but breaking R.5. You will find that as soon as you (correctly) encapsulate the ```fstream``` and ```mutex```, the bug goes away, regardless of how you break R.5. – Mark Wallace Jul 08 '22 at 21:57
  • I agree that CP.50 is sufficient on its own to fix the bug. Our dispute is whether R.5 also applies, which generalizes to outside synchronization. I think in the context of the question it clearly does: the user calls make_unique which is "making a heap allocation". The rule's suggested enforcement flagging local unique_ptr shows that this is their intention too. – Jeff Garrett Jul 08 '22 at 22:17
0

A mutex helps you do multithread programming and, in such a case, you don't have the same stack frame and you don't usually care about any sort of destruction order. When using multiple threads, the order depends on the OS, not the compiler.

A mutex is not the sort of the object you want to own (at the C++ class level), copy or move. You take ownership at the OS level, not at the C++ level and it has no meaning in a single thread (i.e. singe stack frame where the order matters).

It's similar to when multiple threads are waiting for the mutex, you don't know the order that the threads will be released and you shouldn't code based on that.

Michael Chourdakis
  • 10,345
  • 3
  • 42
  • 78