CODEWITHSUNDEEP
c++thread-safety
Rajeshwar
Rajeshwar

Reputation: 11651

Is this practice thread safe?

Say I have a class like this.. This is just an abstract code

class foo
{
public:
  int a;
  static foo* pfoo;
  foo()
  {
    pfoo = this;
  }
  fooCar* pFooCar;
  void someMethod();
}

Now some other class might do this

foo::pfoo->someMethod();

or 

foo::pfoo->a = 14;

Is this practice correct?

Should I make the static variable of the class private and wrap a getter of the static variable in a mutex?

Something like this

class foo
{
private:
static foo* pfoo;
public:
  int a;
  foo()
  {
    pfoo = this;
  }
  static foo* getFoo()
  {
      lock mutex..
      return pfoo
      unlock mutex..
  }
  fooCar* pFooCar;
  void someMethod();
}

Any other suggestions ?

Upvotes: 2

Views: 160

Answers (4)

ECrownofFire
ECrownofFire

Reputation: 462

First of all, your static pfoo is basically just the singleton pattern, so you can replace that pfoo stuff with this(Meyers singleton), which is guaranteed safe (if you want to change the pointer afterwards then this doesn't apply):

class foo {
public:
    static foo* getFoo() {
        static foo singleton;
        return &singleton;
    }
};

However, anything you do after you receive the pointer is not guaranteed thread-safe. But that all depends on what you need from the code. Thread-safety varies a lot depending on the application, particularly on the number of readers and writers.

Now if you only want a single thread to access the foo's members at a time then you can just have a single mutex:

class foo {
std::mutex fooMutex;
int bar;
public:
    static foo* getFoo() {
        static foo singleton;
        return &singleton;
    }

    void setBar(int newBar) {
        std::lock_guard guard(fooMutex); //This goes in every function that modifies the data.
        bar = newBar;
    }

    int getBar() {
        std::lock_guard guard(fooMutex); //If simultaneous readers are allowed then remove this
        return bar;
    }
};

It all depends on your use case. This is just the simplest, most naive, and least efficient method of doing this. This does not work if you need to, for example, change the value of bar depending on its current value.

Upvotes: 1

bvj
bvj

Reputation: 3392

No, you'll need to wrap the assignment to the static member in an atom step; e.g., critical section, mutex, InterlockedExchangePointer. While it seems like such a simple assignment should be thread safe, it's not; i.e., the assignment is not natively atomic potentially exposing a partially assigned multibyte variable to another thread.

EDIT: sample per op's request and dropped confusing paragraph

Of course, you'll need to setup your mutex prior to using it

 foo()
  {
    pthread_mutex_lock (&mutex);
    pfoo = this;
    pthread_mutex_unlock (&mutex);
  }


  static foo* getFoo()
  {
      foo *retval;
      pthread_mutex_lock (&mutex);
      retval = pfoo;
      pthread_mutex_unlock (&mutex);
      return retval;
  }

Upvotes: 0

Deck
Deck

Reputation: 1979

No. Static variable initialization isn't thread-safe itself. In general case you should always use mutexes/spinlocks when you access to shared variables. It's not atomic.

Upvotes: 0

Nik
Nik

Reputation: 1334

Depends on your design requirement. If there are objects of same class working on some shared data across threads, then you would need to explicitly handle the synchronize threads access to shared data.

In such cases exposing member variable via getters and setters with locks would be correct.

Upvotes: 0

Related Questions