[multithreading] Is this code correct

[EzwRko23]

Using garbage collection with C/++ is like trying to have your cake and eat it too.

You never know it if you are a library writer. Some people run code they don't have sources for under BDW to fix memory leaks.

That really is what should be done in C++ as well.
They really didn't include that in C++0x?

They didn't. But fortunately MS VC 2003 treats volatile the same as Java.

The compiler generates thread-unsafe code for code that's thread-unsafe. What a surprise.

A surprise is that noone here except m4ster r0shi spotted this is thread-unsafe, and you, helios, are in this group. So, we have here some "expert" C++ programmers that were not aware of out-of-order processing. Even though I asked "are you sure" and gave hints.

[helios]

A surprise is that noone here except m4ster r0shi spotted this is thread-unsafe, and you are in this group. :D Even though I asked "are you sure" and gave hints.

I wrote:

userManager isn't necessarily aligned and accesses to it aren't necessarily atomic? Although that's a rather platform-specific behavior. Technically, memory accesses don't have to be atomic even if the memory is aligned.

[EzwRko23]

Let me cite you:

As long as the UserManager implements its own locking mechanisms, it will be thread-safe.

Other than the fact that there's nothing preventing any other function from writing to the pointer, I don't see anything else. Certainly nothing to do with concurrency (other than what I already mentioned in my first post, about Lock not knowing what to lock).

So no, you haven't spotted the problem, really.

[Disch]

xorebxebx wrote:

No. The problem of writes committed out of order does not occur, when special instructions are emitted (memory berrier). This is what locking does, so the second code snippet is perfectly thread-safe.

So then if locking ensures that all writes are commited, couldn't you just nest the allocation in another lock?

volatile Foo* inst = 0;

Foo* GetInstance()
{
  if(!inst)
  {
    Lock a(mainmutex);
    if(!inst)
    {
      Foo* temp = 0;
      {
        Lock b(phoneymutex);
    
        temp = new Foo;
      }  // b's unlock ensure's Foo's allocation writes have been commited.
      // however b's lock will never be done by 2 threads, so it is relatively inexpensive

      inst = temp;
    }
  }
  return inst;
}

[helios]

The first quote was in reply to Disch.

However using the returned pointer would not be threadsafe unless it's behind the same mutex again.

Which I'm guessing he meant this

getUserManager()->foo();

wouldn't be thread-safe unless foo() used the same mutex as getUserManager(), which isn't true.

So no, you haven't spotted the problem, really.

I didn't spot that particular problem, but it doesn't matter. Suppose what m4ster r0shi mentioned didn't exist. Does that make the code you posted safe? No. There's still a possible race condition because you don't know if the check is atomic. You don't get to complain about overly aggressive compiler optimizations if you do things you shouldn't do. Like those idiots who complain about this having undefined behavior:

const int a=10;
int *b=(int *)&a;
*b=20;

[EzwRko23]

@Disch, yes you are right - think this should be safe, because you effectively created a write barrier.
@helios, you are also right: This whole double check optimization is broken in languages which don't provide atomic volatile. Anyway, on most 32-bit systems, pointer read and writes of global variables are atomic, so this rarely is an issue. While out of order writes happen very often.

[m4ster r0shi]

xorebxebx wrote:

A surprise is that noone here except m4ster r0shi spotted this is thread-unsafe

Hahaha, don't give me credit for that. I just happened to download that book this morning (and have been reading it since then; I'd say it's very interesting).