4

I am confused with the following code (from Prefer Using Active Objects Instead of Naked Threads):

class Active {
public:

  class Message {        // base of all message types
  public:
    virtual ~Message() { }
    virtual void Execute() { }
  };

private:
  // (suppress copying if in C++)

  // private data
  unique_ptr<Message> done;               // le sentinel
  message_queue< unique_ptr<Message> > mq;    // le queue
  unique_ptr<thread> thd;                // le thread

private:
  // The dispatch loop: pump messages until done
  void Run() {
    unique_ptr<Message> msg;
    while( (msg = mq.receive()) != done ) {
      msg->Execute();
    }
  }

public:
  // Start everything up, using Run as the thread mainline
  Active() : done( new Message ) {
    thd = unique_ptr<thread>(
                  new thread( bind(&Active::Run, this) ) );
  }

...

(After constructor completion member variables are accessed only in the member thread).

Could somebody explain me the reasons why in this case it is safe to share Active object's this between threads? Do we have any guarantees that the new thread will see the changes made in the constructor before the thread spawn line?

Toby Speight
  • 27,591
  • 48
  • 66
  • 103
amir.tu
  • 83
  • 5
  • How is `Active` supposed to be used by multiple threads? I cannot see any code that would allow doing *anything* with it. – 5gon12eder Jan 29 '16 at 15:04
  • 1
    It is technically UB. But always works in practice because any real OS or threading support library must pass a memory barrier to context-switch. – Hans Passant Jan 29 '16 at 15:07
  • @5gon12eder Trying make code clearer omitted some methods, please follow http://www.drdobbs.com/parallel/prefer-using-active-objects-instead-of-n/225700095?pgno=2 if you are interested. – amir.tu Jan 29 '16 at 15:11
  • 1
    @HansPassant, there is at least implicit guarantee of this, though, since without it you would not be able to pass any arguments to threads at all. I believe, I've seen an implicit gurantee of threads creating and joining being a barrier, but I do not remember where. – SergeyA Jan 29 '16 at 15:21
  • @HansPassant What exactly is UB here? Do you see a data race? – 5gon12eder Jan 29 '16 at 15:22
  • To add to Hans statement, you could face problem with pure user-space green threads library doing std::threads job. AFAIK, no such library is in widespread use – Severin Pappadeux Jan 29 '16 at 15:26
  • @SergeyA Thank you for the hint, found similar question: http://stackoverflow.com/questions/17192991/c11-when-to-use-a-memory-fence – amir.tu Jan 29 '16 at 15:36
  • @5gon12eder, my standarteeze is too weak to understand if this means laymen barrier or not. – SergeyA Jan 29 '16 at 15:39
  • 1
    It is UB because the program does not have explicit synchronization. It implicitly relies on the worker thread having an up-to-date view of the mq and done variables. An assumption like this is *very* common, lots of code would break if thread context switches don't imply a memory barrier There's another trap hidden in this code btw, it is also not legal to dereference *this* until the constructor finished executing. Code gets away with it by not having virtual members. – Hans Passant Jan 29 '16 at 15:39
  • @HansPassant, while I am not sure about barrier, I am 100% sure you are wrong about dereferecning `this`. This is perfectly legal to dereference this within the constructor, it just points to the class being constructed. – SergeyA Jan 29 '16 at 15:41
  • 1
    @HansPassant § 30.3.1.2 ¶ 6 says: *“The completion of the invocation of the constructor synchronizes with the beginning of the invocation of the copy of `f`.”* (I've just commented this before but deleted it again because it is redundant with the answer linked by @amir.tu.) I'm also quite sure that dereferencing the `this` pointer in a c'tor is not UB. – 5gon12eder Jan 29 '16 at 15:49
  • 1
    Key point is that *this* is being dereferenced in code that is not part of the constructor and executes completely out-of-sync with constructor execution. Virtual members may not yet be virtual. Or may be, completely depends on timing. Reasoning how that could produce failure in this case isn't very productive since the class does not have virtual members. Well, that we can see anyway. – Hans Passant Jan 29 '16 at 15:52
  • 1
    @HansPassant: It's not _completely_ out of sync, since the thread cannot come into existence before the member variables are constructed. The class intentionally has no virtual members--it's intended as a helper you include by composition, not inheritance. – Adrian McCarthy Jan 29 '16 at 18:18

2 Answers2

2

I believe this is safe (no data races) because of technicalities that are not made explicit in the code.

The thread may start before the Active object is fully constructed, which sounds like it could be a problem because the spawned thread could access the vtable or other member variables.

However,

  1. There is no vtable, since Active is not a base class nor is it designed to be. Active is designed to be used with composition, not inheritance.

  2. The thread is not spawned until the body of the constructor, which means all the member variables have been fully constructed. (C++ constructs the members in the order they are declared, and you can be assured that a previously-declared member variable is fully constructed before the next one is constructed.) By the time the thread is spawned, the done flag and the queue are fully constructed. Since the thread never accesses the thd member, there's no race there with the main thread completing the assignment.

  3. Once the worker thread is spawned, the owner thread never touches the object again, other than perhaps finishing the assignment to thd. But the spawned thread never accesses thd, so there is no race there.

  4. The worker thread sets its own done flag (in response to a message), so there's no race there.

  5. I presume the message queue has the necessary synchronization.

So I believe this is fine.

Adrian McCarthy
  • 45,555
  • 16
  • 123
  • 175
  • Basically agree, but at least we also need to consider compiler/processor instruction reordering and register caching (are there any other issues we should be aware of in this code?). – amir.tu Jan 29 '16 at 19:02
  • @amir.tu: I can't see a way for reordering or register caching to make a difference here. Can you provide an example? If a barrier is required to ensure the arguments are visible the new thread, that's the responsibility of of the std::thread constructor (or thread arguments would never be reliable). The issue here is that we're passing a pointer to an object that may not be fully constructed, but I argue that it must be constructed enough by the time the thread is created that it cannot make a difference. (I'm open to the possibility that I'm wrong. Persuade me.) – Adrian McCarthy Jan 29 '16 at 19:19
  • May be it is not only about thread arguments to be reliably visible, but about everything before thread constructor invocation. – amir.tu Jan 29 '16 at 20:16
1

It is safe to share the this pointer since the std::thread in question is owned by the object.

This however assumes that the thread object is joined in the destructor (or prior to destruction). If std::thread::detach is called for instance, and the Active object destructs, the behavior becomes undefined.

lcs
  • 4,227
  • 17
  • 36