Is it wise to lock a mutex to just return a value?

Question

class Foo {
public:
    // ...
    const int &getBar() const noexcept;

    void doSomethingWithBar(); // (2)

private:
    std::mutex barMutex;
    int bar = 7;
};

const int &Foo::getBar() const noexcept {
    std::lock_guard<std::mutex> lock(this->barMutex); // (1)
    return this->bar;
}

void Foo::doSomethingWithBar() {
    std::lock_guard<std::mutex> lock(this->barMutex); // necessary here
    this->bar++;
}

In terms of thread-safety, is line 1 necessary, considering that another thread might interfere and call the function in line 2 and thus change the value of bar?

Note that int might be any type here.

You are worrying about the wrong thing. Line (1) neither helps nor hurts, since the next line never reads the value of `bar`. The race condition will happen in the calling code, which would presumably read the value via the reference you return, with no protection. — Igor Tandetnik, Aug 17 '14 at 18:35
@Igor that's wrong, line 1 gives us important visibility guarantees! — Voo, Aug 17 '14 at 18:39
Also, it is almost always pointless to return a `const int &`. — jxh, Aug 17 '14 at 18:41
@Igor What if `int` was a class here (which is completely thread-safe itself); would returning the const reference still be dangerous? @jxh: As mentioned above, the int is just sort of a place-holder for any type. — , Aug 17 '14 at 18:42
@MartinD. see my answer for more detail, but regardless of what type the reference points to, a reference is a reference. — OMGtechy, Aug 17 '14 at 18:44
If the class is completely thread-safe, then what is the purpose of `barMutex`? Why add a layer of external synchronization to a class that already performs the necessary synchronization internally? — Igor Tandetnik, Aug 17 '14 at 18:47
@Igor locking the mutex gives us sequential consistency by default which if we made a copy would actually make the code correct. Not the best way to do that though obviously. — Voo, Aug 17 '14 at 18:57
@Voo `which if we made a copy...` If the code looked differently, then my comments would look differently, too. If you want to argue over some piece of code other than the one shown, I suggest you post a new question, showing that code. — Igor Tandetnik, Aug 17 '14 at 19:02
@Igor You claimed line 1 had no effect whatsoever.that's like saying `printf("hello world")` had no effect here because of some other bug. There are valid code paths that don't lead to undefined behavior but only if we keep the lock. — Voo, Aug 17 '14 at 19:09
@Voo: what are these code paths? Could you be more specific? I claim that the observable behavior of this code (as written), or any possible additional code not shown, wouldn't change at all if line (1) were removed. Any code that exhibits undefined behavior with (1) removed would also exhibit undefined behavior with (1) intact. Please show an example that proves me wrong. — Igor Tandetnik, Aug 18 '14 at 00:32

score 4 · Accepted Answer · edited May 23 '17 at 11:58

4

Seeing as you're returning a reference, locking is entirely useless for you in this scenario. You may want a lock when you use the reference that is returned though.

However, if you were returning a value it will have more of an effect, take a look at this for an example of a torn read.

edited May 23 '17 at 11:58

Community

1
1

answered Aug 17 '14 at 18:37

OMGtechy

7,935
8
48
83

3

Locking a mutex on write only, but not on read, will result in a race condition, since it allows a read at the same time a write is occurring. So no, this doesn't solve any problems. – Igor Tandetnik Aug 17 '14 at 18:44
@IgorTandetnik thanks for pointing that out, I got it mixed up with std::atomic. Will edit the answer. – OMGtechy Aug 17 '14 at 18:45

Christophe · Answer 2 · 2014-08-17T18:56:18.403

Line 1 has not the effect that you expect: apparently, you try to lock the object and keep it locked while the caller works on the reference returned.

But the lock_gard you create is a local object of getBar() and gets destroyed as soon as you return, thus unlocking the lock that you just acquired.

Several alternatives, for example:

You may change your function to return the value of bar, but taking into consideration that this value is a snapshot that can be obsolete when you'll use it.
You may also change your function to get and process bar in one shot (for example by providing a function as parameter).
You could also manage the lock as class member and provide a function releaseBar() to unlock the mutex as soon as it isn't needed anymore. This is however is a more dangerous, approach, because the caller may forget to release the lock.

Is it wise to lock a mutex to just return a value?

2 Answers2