0

So, I have the following two functions in THE SAME class:

void start()
{
...

 m_pRunnable = createRunnable(
         [spDiskManager = m_spDiskManager,
          path = m_path,
          &success = m_isFolderCreated]() {
            success = spDiskManager->createDirectory(path);
         });


m_spWorkThread->invokeWithDelay(m_pRunnable.get());
}


handle(ThreadDoneEvent& e)
{
   if(m_isFolderCreated)
   {...}
}

In start(), the m_pRunnable is a member variable, which is assigned a lambda functions with the variable m_isFolderCreated captured by reference. The reason for this is that it returns the result of the spDiskManager method createDirectory, and will later be used in the function handle(ThreadDoneEvent& e) to check if a folder was created or not. The lambda will be executed in a separate thread, not in the thread the class executes in. Note that handle(ThreadDoneEvent& e) will be called only AFTER the worker thread finishes execution, since the event ThreadDoneEvent will only be published when the worker thread has finished executing.

My question is: In this particular case, and in general, would you consider the capture of a member variable by reference, an ok practice, when to be used in another thread like here?

I mean, it is meant to only be used as a read-only variable in the class, but once someone else chooses to do changes in this class(which in reality happens to big quite big), there is a small chance that he will try to change this variable and we can get a race condition.

In other cases, you may want to capture more variables by reference for your lambda to execute, which seems even more dangerous. Another way of doing this would be to have a separate class which will hold the success boolean and run the function createDirectory through that class. Once it has run, you could extract the success bool, set by createDirectory, through a getter. But that would be more code to write...

2
  • 3
    The only thing of concern in single-threaded code when handing out references is ensuring the object stays alive long enough. Add asynchronous callbacks, and re-entrancy is added. Add MT, and race-conditions are always a concern. Seems you got that mostly handled, but I think the member would need to be std::aromic<>. Nov 24, 2021 at 22:47
  • Not sure what you mean by "read only variable" since you assign to it on the very next line after the capture! Also to do this obviously means the instance has to stay alive until the thread is finished - the thread itself won't keep it alive - so since that's the case why not capture this instead? At least it will make that obvious.
    – davidbak
    Nov 26, 2021 at 14:53

1 Answer 1

2

There are two concerns here:

  • Is there a use-after-free? You need to ensure that the class instance survives long enough for the thread to finish. C++ will generally not help you here, but you could, for example, put an assertion inside the destructor that m_pRunnable has finished.
  • Is there a race condition? Any use of a variable that could concurrently be written by a different thread is a data race and thus undefined behavior. You need to ensure that this doesn't happen. Again, probably your best chance is to guard any access to m_isFolderCreated with checking that m_pRunnable has finished, either by asserting on it, or falling back to some different behavior. Alternatively, you can my m_isFolderCreated atomic, so that concurrent access is allowed.

Finally, I would put some comments on the relevant member variables that document the assumptions about the access from multiple threads.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.