0

I had a problem with starting and stopping background threads based on the value of a boolean flag. I had asked for help here and got a very detailed answer here.

However, after using the solution for a few days, it seems to me that there is something wrong with it. I launch a worker thread in SomeObject::DoRun called in SomeObject::Start which should do some work in a loop and quit when I call SomeObject::Stop:

struct SomeObject {
  SomeObject() { SetDone(true); }
  std::atomic_bool done_;
  void DoRun();
  void SetDone(bool v) { done_store(v); }
  bool IsDone() { return done_.load(); }
  void Start() {
       if (IsDone()) {
           SetDone(false);
           DoRun();
       }
  void Stop() { SetDone(true); }
};

However, it seems like sometimes the check for IsDone() in Start() returns false and DoRun() never gets called even though I have set done to true in the constructor.

The reason I set done_ to true in constructor and then set it to false in Start() is that I want to call Start()/Stop() repeatedly on the same object.

What am I doing wrong here?

Community
  • 1
  • 1
341008
  • 9,522
  • 11
  • 48
  • 84
  • 1
    Probably unrelated problem: `Start` appears to try to make sure that only one thread executes `DoRun`, however, two threads can execute `Start` at the same time, both reading true from `IsDone`, both calling `SetDone(false)` and then both calling `DoRun`. You can use `compare_exchange` to fix that. Also `return done_.load();` is redundant, may as well write `return done_;`. Similarly `done_.store(v);` can be replaced with `done_ = v;` to do the same thing. – nwp Jun 03 '14 at 17:53
  • @nwp `Start` is called on the main thread only. – 341008 Jun 03 '14 at 18:27
  • I'm guessing that this is not your actual code because it has syntax errors - e.g. `done_store()` instead of `done_.store()` and `Start()` braces aren't balanced. The problem is probably in your real code. I do see one problem in the logic - presumably your worker thread polls `IsDone()` periodically to determine if it should exit. You can have two worker threads running simultaneously on the same object after the sequence `Start`/`Stop`/`Start` because the thread from the first `Start` may not notice the new state until the thread from the second `Start` is already going. – rhashimoto Jun 03 '14 at 18:51
  • How did you determine that `DoRun()` isn't called? Also, are you talking about the first `Start()` after the construction of `SomeObject` that failes or is this happening later after some Start/Stop cycles as rhashimoto explained? – MikeMB Jun 03 '14 at 21:58
  • Is there any mechanism that tells your main thread that `DoRun()` stopped? Maybe you should also show the callsite in your main thread. – MikeMB Jun 03 '14 at 22:01
  • @rhashimoto I have not called `Start` twice yet, so there can't be two threads – 341008 Jun 04 '14 at 01:23
  • @MikeMB I know DoRun is not called because I set a breakpoint and that was not hit. Also, this is happening at the first `Start`. – 341008 Jun 04 '14 at 01:25
  • It should note make a difference, but have you tried initializing done_ in the initializer list? Also, just to make sure: the only place where 'done' is set to false is in 'Start()' and 'Start()' is called in the same thread, in which 'SomeObject' is created-right? – MikeMB Jun 04 '14 at 07:11
  • @MikeMB I cannot initialize `done_` in initializer list because of this issue: http://connect.microsoft.com/VisualStudio/feedback/details/720151/c-11-unsupported-std-atomic-bool-constructor. Yes, to your other questions. – 341008 Jun 05 '14 at 05:23
  • Then I can only think of three more things: either your real code has a (probably trivial) nug, which is not reflected in your example or my understanding of atomic_bool is wrong or you found a bug in the VS C++ compiler. I know, its difficult for multithreaded programms, but can you provide a SSCCE? As a possible workaround (depending on the actual reason for this bug) you might want to try std::atomic_flag. – MikeMB Jun 05 '14 at 11:23

0 Answers0