CODEWITHSUNDEEP
c++multithreadingshared-ptrrace-conditionthread-synchronization
Pulkit Sharma
Pulkit Sharma

Reputation: 324

Returning shared_ptr in multithreading is safe?

I have just started working in multi-threaded environment with c++.I have doubt in below code snippet.

std::shared_ptr<SomeInterface> getSessionLocked(const std::string& token) {
          std::lock_guard<std::mutex> guard(m_mutex);
           if (!findSessionByToken(token)) {
               return nullptr;
          }
          return m_activeSessions[token]; 
    }
bool findSessionByToken(const std::string& token) {
      return !(m_activeSessions.find(token) == m_activeSessions.end());
 }

Both m_mutex and m_activeSessions are private variables.Now my question is that "Is it safe to return shared pointers like this" because i think there might be possibility of data inconsistency because of race conditions. Can someone clear this thing ?

Upvotes: -1

Views: 171

Answers (1)

ichramm
ichramm

Reputation: 6642

This funcion:

std::shared_ptr<SomeInterface> getSessionLocked(const std::string& token) {
      std::lock_guard<std::mutex> guard(m_mutex);
       if (!findSessionByToken(token)) {
           return nullptr;
      }
      return m_activeSessions[token]; 
}

is ok because:

  1. You don't read from m_activeSessions unless you have the lock
  2. You return std::shared_ptr, so the object won't be destroyed until all reference to this shared pointer go out of scope (even if you remove the token from m_activeSessions.

One think that could be better/you need to watch for:

  1. Consider the rate #readers / #writers in order to see if you need a reader-writer lock.
  2. Consider using std::weak_ptr if think it is difficult to track all the different places where the shared pointer might be used.

On the other hand, this function:

bool findSessionByToken(const std::string& token) {
    return !(m_activeSessions.find(token) == m_activeSessions.end());
}

Is not thread safe (the iterator returned by find could be invalidated before the comparison takes place).

Now, in your example the function is called with the lock held so: is this function public or private?

If this is a private function then it's ok, but I would recommend changing the name to reflect that the fact that is not thread safe (something with the Unlocked or Unsafe suffix).

If this function is public then you have a problem.

Finally, the function returns bool so the name should be something like containsToken or containsTokenUnsafe. The word find indicates that you are going to return the session, but you didn't.

A final thought:

Considering this is a multi-threaded app, where concurrent access to shared resources is expected, I would make concurrency a first class citizen an change the nomenclature:

  • Change getSessionLocked to getSession. Not only because the reason above but also because the caller shouldn't care whether you use a lock or not. I mean, how would an external user handle non-protected access to a class private data?
  • Make non thread-safe functions at least private (adding the unsafe/unlocked suffix may be a personal preference).

Upvotes: 1

Related Questions