5

Suppose I have a function that tries to protect a global counter using this code:

 static MyCriticalSectionWrapper lock;
 lock.Enter();
 counter = ++m_counter;
 lock.Leave();

IS there a chance that two threads will invoke the lock's constructor? What is the safe way to achieve this goal?

Alex Emelianov
  • 1,254
  • 10
  • 10

4 Answers4

4

The creation of the lock object itself is not thread safe. Depending on the compiler, you might have multiple independent lock objects created if multiple threads enter the function at (nearly) the same time.

The solution to this problem is to use:

  • OS guaranteed one time intialization (for the lock object)
  • Double-checked locking (Assuming it is safe for your particular case)
  • A thread safe singleton for the lock object
  • For your specific example, you may be able to use a thread safe interlocked (e.g., the InterlockedIncrement() function for Windows) operation for the increment and avoid locking altogether
Michael Goldshteyn
  • 71,784
  • 24
  • 131
  • 181
  • Then, to make it safe, I need to wrap it in a critical section, leading to the same problem... right? – Alex Emelianov Dec 30 '10 at 22:27
  • 1
    A thread-safe singleton is harder to write than you might think. A better solution is to eliminate the global and use an active object instead. The active object will then be in charge of whatever action you intended to do with the global. – wilhelmtell Dec 30 '10 at 22:30
  • 5
    AFAIK, Double-checked locking is not considered thread-safe. It's kind of anti-pattern. – Piotr Findeisen Dec 30 '10 at 22:31
  • Sometimes you need a singleton assigning unique IDs. It should not be that hard to implement. – Alex Emelianov Dec 30 '10 at 22:32
  • @AlexEmelianov yes, it is that hard to implement **correctly**. If you introduce the singleton in order to bring in simplicity then the solution defeats itself. – wilhelmtell Dec 30 '10 at 22:33
  • @Piotr, that depends on the language, OS and implementation of said pattern. I offered it as an option, because under the right conditions (e.g., Windows and C++) it can be a valuable aid, as the linked Wikipedia article explains. – Michael Goldshteyn Dec 30 '10 at 22:33
  • @wilhelmtell: Think of it as something you do to fix existing code. The call is ten levels deep, introducing new parameters or refactoring is possible, but you have to establish the correct behavior now. – Alex Emelianov Dec 30 '10 at 22:36
1

Constructor invoke can be implementation and/or execution environment dependent, but this isn't a scoped_lock so not an issue.

Main operation is properly guarded against multi thread access I think.

(You know, global for global, function static for function static. That lock variable must be defined in the same scope with the guarded object.)

9dan
  • 4,222
  • 2
  • 29
  • 44
0

Original sample code:

 static MyCriticalSectionWrapper lock;
 lock.Enter();
 counter = ++m_counter;
 lock.Leave();

I realize that the counter code is probably just a placeholder, however if it is actually what you trying to do you could use the Windows function "InterlockedIncrement()" to accomplish this. Example:

 // atomic increment for thread safety
 InterlockedIncrement(&m_counter);
 counter = m_counter;
Chris Bennet
  • 587
  • 5
  • 13
-1

That depends on your lock implementation.

wilhelmtell
  • 57,473
  • 20
  • 96
  • 131
  • Please explain. You can assume straightforward wrapper of CRITICAL_SECTION with `Init()` in the constructor, and `Enter()` and `Leave()` simply delegating to Windows API. With a conditional compile directives to branch to other OS implementations, but that's not the point. – Alex Emelianov Dec 30 '10 at 22:30
  • -1 I don't think that your statement is correct. Initialization (construction) of `lock` is done when the function is first called, but if two threads both call the function at the same time before the `lock` variable is initialized, then each thread might succeed in creating its own lock. Then, regardless of the correctness of the lock implementation, both threads might execute the critical code at the same time. – Daniel Trebbien Dec 31 '10 at 00:04
  • @DanielTrebbien No. If the lock is properly written then two threads will **not** be able to acquire it at the same time. That is the whole point of the lock to begin with, and if it doesn't fulfil it then it's not a lock but a bug. A properly written C++ lock will acquire at construction and release at destruction. Then, there's no need for the subsequent `lock.Enter()` and `lock.Leave()` calls. That's RAII. If the lock's ctor fails to acquire then it should throw. The ctor must have an atomic section for this to work. – wilhelmtell Dec 31 '10 at 00:10
  • It's not "regardless of the lock's implementation". The lock's constructor **is** the lock's implementation. **That's** where the lock must be correct. Once constructed the lock does roughly nothing. The only thing left is to write a proper destructor, that releases the lock. – wilhelmtell Dec 31 '10 at 00:16
  • 1
    @wilhelmtell:Sorry,I wasn't clear. Here, `lock` is a static block-scope variable. Section [6.7] paragraph 4 of the (current) standard says that "such an object is initialized the first time control passes through its declaration". http://stackoverflow.com/questions/898432/how-is-static-variable-initialization-implemented-by-the-compiler covers how C++ compilers do this. It's not thread-safe, so multiple threads *could* simultaneously run the `MyCriticalSectionWrapper` constructor, leaving it in an undefined state. Your statement is correct, though, for C++0x based on the latest draft, n3126. – Daniel Trebbien Dec 31 '10 at 00:16
  • @DanielTrebbien the critical section is inside the ctor. It's ok for two threads to get in. What's important is for one thread at a time to get out. A thread can leave the ctor only if the lock is released. This critical section management is the tricky part. There are lots of papers out there on how to get it right. – wilhelmtell Dec 31 '10 at 00:18
  • 1
    @wilhelmtell: "MyCriticalSectionWrapper" is a C++ class. Even if the internals are written correctly, two different instances don't have the same behavior as one instance. Two threads, creating two different instances, would (correctly, but pointlessly) lock two different contexts. My question was if the "static" declaration somehow prevented double initialization. – Alex Emelianov Dec 31 '10 at 00:19
  • @AlexEmelianov You should really read concurrency papers. Find a good book, there are a few. One technique is to pass a value to the destructor, so that the lock is a function of the value. Then two locks compare based on this value. – wilhelmtell Dec 31 '10 at 00:21
  • @AlexEmelianov Herb Sutter is one of the famous proponents of working with locks to begin with. One of his strong arguments against locks is that locks are hard to get right, and even once you do get them right their distributed nature are a call of bugs. That is, users have access to the resource *and* to the lock, and they're expected not to use the resource without the lock. If they do, it leads to obscure bugs. See http://www.drdobbs.com/go-parallel/article/showArticle.jhtml;jsessionid=KIYJDIOMHBU3BQE1GHOSKH4ATMY32JVN?articleID=227500074 – wilhelmtell Dec 31 '10 at 00:25
  • 1
    @wilhelmtell: please don't assume I don't know the basics. You might be surprised how much concurrent code I have debugged... At this point, I simply needed proof that some existing code is not as safe as the team owning it believes. It always helps to use constructive language rather than "read a book first" kind of remarks. – Alex Emelianov Dec 31 '10 at 00:26
  • @AlexEmelianov sorry if I hurt you. What I meant is that your (and everyone elses' here) assumption that the code is incorrect is plain wrong. You can downvote me all you want, and even tell me you are hurt after that, but the point stands. If that was incorrect regardless of the lock then it wouldn't be possible to write concurrent C++ code. **Of course** two threads can enter a constructor. **So what?** – wilhelmtell Dec 31 '10 at 00:29
  • @wilhelmtell: if the two constructors operate on the same critical section, it would be OK. If two instances of `myCriticalSectionWrapper` wrap two different critical sections (as is the case in my real code), the behavior is not right. – Alex Emelianov Dec 31 '10 at 00:32
  • 1
    @wilhelmtell `lock` is not the lock object, but the critical section instance itself. This is an unfortunate naming, as `lock` should have been named `sync_object`, or something similar. The concern of the others is that the critical section or mutex object cannot be locked until it is properly created first. – Tamas Demjen Dec 31 '10 at 01:02
  • 2
    Apparently C++0x is going to guarantee thread safety with static locals, as Daniel's link suggests: http://stackoverflow.com/questions/898432/how-is-static-variable-initialization-implemented-by-the-compiler. This guarantee is missing from C++2003. The safest approach is to initialize all critical sections in main (before you even create your first thread), but that's inconvenient. The idea of the static local is that it would create the critical section instance the first time it's needed (on demand). But without the thread-safety guarantee it is implementation dependent. – Tamas Demjen Dec 31 '10 at 01:20
  • @Tamas Demjen: this is exactly the answer I was looking for. Sorry that the example was kind of misleading. – Alex Emelianov Dec 31 '10 at 01:30