Is this safe across threads?

Question

I have some code that looks like this:

Thread 0:

CMyOtherClass *m_myclass;

CMyClass::SomeFunc(DWORD SomeParam)
{

m_myclass = new CMyOtherClass(SomeParam);

}

Thread 1:

CMyClass::InsideSecondThread()
{

   MySecondThreadFunc(*m_myclass);

}

CMyClass::MySecondThreadFunc(MyOtherClass& myclass)  
{

// do something with myclass variable....
// What about thread safety???  Is this not now a shared variable?
// or have we passed a copy and are we safe?
// SomeParam should be the same while in this thread, however it can be changed by Thread 0


}

So my question is, if you are passing this m_myclass variable across threads, is this threadsafe or not?

score 4 · Answer 1 · answered Jun 28 '10 at 08:03

4

It is not threadsafe.

answered Jun 28 '10 at 08:03

TBH

451
2
9

Explain further. There are no conflicting accesses to any variable in his code. – avakar Jun 28 '10 at 08:06
There is a conflict. CMyClass::SomeFunc() is creating an object ("m_myclass = new CMyOtherClass();") And the CMyClass::MySecondThreadFunc(MyOtherClass& myclass) is doing _something_ with it. It is NOT threadsafe. – TBH Jun 28 '10 at 08:09
My bad, I skipped the comment as always and failed to understood the question correctly ("can you pass variables across threads", which you certainly can). – avakar Jun 28 '10 at 08:16
@Jurily, this has nothing to do with `new`. It's the assignment that's not atomic. – avakar Jun 28 '10 at 08:17

score 2 · Answer 2 · answered Jun 28 '10 at 08:07

2

It is not threadsafe. If thread 1 gets executed before the thread 0 creates the object then you end up in accessing NULL pointer in thread 1. ( Assuming m_myclass is initialised to NULL in the constructor)

Another point is :

CMyClass::MySecondThreadFunc(MyOtherClass& myclass)

the myclass object is a shared object and any operations from different threads can create problem. Unless there is a shared static member, local copy should work for thread safe here.

answered Jun 28 '10 at 08:07

aJ.

34,624
22
86
128

m_myclass is not initialized in the ctor. Just declared... as a pointer. – Tony The Lion Jun 28 '10 at 08:10
then might end up in accessing garbage value stored in the myclass – aJ. Jun 28 '10 at 08:12

score 0 · Answer 3 · edited May 23 '17 at 12:11

0

It's not threadsafe because both threads are accessing the same bit of memory.

As your code stands, as long as thread 0 allocates the memory before thread 1 starts then you should have no problems because thread 0 doesn't do anything except the allocation. If you change the thread 0 code so it starts modifying the class instance then you'll start to run into issues.

[Edit] Removed my example of what I thought ought to be thread-safe. There's another post here on SO which asks whether changing a pointer is considered an atomic action which is worth a read.

edited May 23 '17 at 12:11

Community

1
1

answered Jun 28 '10 at 08:14

Jon Cage

36,366
38
137
215

thread 0 actually modifies two parameters on construction of the object – Tony The Lion Jun 28 '10 at 08:25
Well as long as that's done before thread 1 starts you're still safe. For future reference, it's worth posting as complete an example as possible if you want the most accurate answers. – Jon Cage Jun 28 '10 at 08:42
1

@Jon Cage, the program you posted is very wrong as it contains data race. You need to synchronize accesses to variables. – avakar Jun 28 '10 at 09:01
I had always thought pointers were threadsafe but from the sounds of things I've just been lucky thus far - ouch! Thanks for the correction avakar. – Jon Cage Jun 28 '10 at 09:22
1

Pointers operations are thread-safe, but they aren't necessarily synchronized with what you do to the target unless you use a memory barrier. – Ben Voigt Jun 28 '10 at 12:57
You mean because the compiler might re-order the operations in an attempt to optomise? – Jon Cage Jun 28 '10 at 17:44
No, because the CPU might reorder operations so it isn't waiting on the memory controller. Worse, it might use different orderings on successive passes through the code, depending on what is already in L1 data cache, making missing memory barriers very difficult to troubleshoot. – Ben Voigt Jun 29 '10 at 03:50

score 0 · Answer 4 · answered Jun 28 '10 at 12:37

0

You can create a new CMyOtherThreadSafeClass which has two members CMyOtherClass *m_myclass and a CRITICAL_SECTION cs. You should InitializeCriticalSection or better with InitializeCriticalSectionAndSpinCount function in the CMyOtherThreadSafeClass constructor and use EnterCriticalSection and LeaveCriticalSection (see http://msdn.microsoft.com/en-us/library/aa910712.aspx and http://msdn.microsoft.com/en-us/library/ms686908.aspx) everywhere if you need access CMyOtherClass *m_myclass member (read or modify, allocate or free).

You can include a method in CMyOtherThreadSafeClass to make easier to use of EnterCriticalSection and LeaveCriticalSection. If all threads of your program will use EnterCriticalSection and LeaveCriticalSection then your program will be thread safe.

answered Jun 28 '10 at 12:37

Oleg

220,925
34
403
798

Yuck. If you want to be platform-specific, since Visual C++ 2008 (maybe even 2005?) all `volatile` writes introduce a memory barrier, so on Windows it's sufficient to make the pointer volatile. – Ben Voigt Jun 28 '10 at 12:59
@Ben Voigt: Sorry I don't understand your comment. Of cause usage of `volatile` can be helpful to eliminate the caching and be sure that all threads always read from the memory and makes no additional optimization assumptions. But `volatile` makes objects not thread safe. I don't wrote a full implementation of `CMyOtherThreadSafeClass`, but the usage of `CRITICAL_SECTION` I see as required. It is possible to use other synchronization object, but Critical Section Objects is the best and the most quick choice in my opinion. – Oleg Jun 28 '10 at 13:15
As @Ben just said, on Visual Studio, `volatile` also enforces a memory barrier, so under that specific compiler, volatile *is* sufficient in that specific case. – jalf Jun 28 '10 at 14:11
@jalf & Ben: I see you interpret the problem from the question only as a problem to making a pointer thread safe. I don't. I am do for the usage of `volatile` for the pointer, but I doubt that this thing along solves the problem, which Tony has in his real code. It is not clear for me where the data from `m_myclass` will be freed and where and when it will be thread safe initialized. So I strictly recommend to use critical sections. If they will be not really needed, then it will took very little resources overhead. – Oleg Jun 28 '10 at 15:42

score 0 · Answer 5 · answered Jun 28 '10 at 14:00

If you call CMyClass::SomeFunc only prior to the second thread being created, then there is no race condition.

Otherwise, it's basically the same problem that prevents double-checked locking from working: The pointer could be assigned to m_myclass prior to the object actually being constructed, and the thread then uses an unconstructed (or partially constructed) object.

Is this safe across threads?

5 Answers5