CODEWITHSUNDEEP
c++multithreadingthread-safety
amnesia
amnesia

Reputation: 1988

C++ shared mutex with return by reference

I have a question that is related to, but not answered by, this question:

Example for boost shared_mutex (multiple reads/one write)?

I understand how the exclusive locks work when the operations are scoped within the member function. My question is, what about return-by-reference functions? Consider the following (psuedo-code):

class A {
    shared_mutex _mutex;
    std::string _name;

public:
    const std::string& name() const {shared_lock(_mutex); return _name;}
}

Then assume I do something like this in code:

A obj;
if (obj.name().length() >0) { ... };

My gut tells me this may not be thread safe, since the mutex will already have gone out of scope by the time the length() function is called, but I don't know.

I guess I'm also asking in the big picture, if I'm trying to make the object thread-safe, should I avoid returning by reference altogether? Wouldn't it enable someone to do something like this:

A obj;
std::string& s = obj.name();
(at this point lock is out of scope)
s = "foo";  // does this change the original object's member since it is a reference?

Upvotes: 4

Views: 3073

Answers (3)

Wandering Logic
Wandering Logic

Reputation: 3403

Presumably you are using a shared_mutex (instead of a plain mutex) for performance reasons, but copying the string is going to be expensive. Also presumably the code inside the braces of if (obj.name().length() >0) { ... } is going to further manipulate the string. If this is the case then you need to acquire the mutex for the entire operation, otherwise you will run into race conditions. (For example, you copy name, take its length, then in the meantime someone else shortens name to have length 0. The code in the braces will now be making an incorrect assumption about the length of name.)

If possible, you might consider making the code that is going to manipulate the string a member function of class A.

class A {
  shared_mutex _mutex;
  std::string _name;

public:
   rettype my_operation() {
     shared_lock lk(_mutex);
     if (_name.length() >0) { ... };
   }
};

There are other options, but they all involve holding the shared lock for the entire period of time that you are depending on your invariant holding.

Upvotes: 1

hmjd
hmjd

Reputation: 121961

It is not threadsafe as the mutex would be released after name() has returned and another thread could begin modification of _name before or during the call to std::string::length() or after and the state of _name would have changed before entering the {...} true branch of the if.

To make the object threadsafe, ensure that all accesses to the _name occur when the mutex is locked. This means ensuring that a reference to _name is not returned to a caller or is not passed to a callback provided to (an imaginary member function not posted in the original code) a member function of A as this callback could cache the address of _name for future, unsynchronized, use.

Instead, return, or pass, by value (and create a copy of _name when the mutex is locked):

// Must be mutable to be modifiable
// in a const member function.
mutable shared_mutex _mutex;

std::string name() const
{
    // Must not create a temporary shared_lock,
    // otherwise the lock will be released immediately.
    // So give the shared_lock a name.
    shared_lock lk(_mutex);
    return _name;
}

Upvotes: 3

Drew Dormann
Drew Dormann

Reputation: 63755

Just change this function delaration:

const std::string& name() const ...

To this.

std::string name() const ...

You'll then be returning a copy of the string. (And since it's a copy, it doesn't need to be const).

That code will be thread-safe, and any compiler optimizations would preserve that safety.

Upvotes: 2

Related Questions