2
\$\begingroup\$

Is everything OK with this code?

I need this mechanism:

  • I want to initialize global object of type A (named 'gA'), which may take very long time
  • I don't want to wait for gA to be initialized, I want to start other threads at the same time (in order for the program to be responsive)
  • Other threads also take some time to initialize their own stuff, and when they are ready, they want to use the gA. It can happen at any moment, before or after gA is initialized. I must assume that other threads are ready before gA is initialized and must wait therefore wait for gA.

I came up with this simple solution using shared_future. The code works and does what I want. I only wonder if there are some dangers hidden within this approach. Maybe you can spot some bug in this code?

#include <iostream>
#include <chrono>
#include <memory>
#include <future>
#include <functional>

using namespace std;
using namespace chrono_literals;

struct A
{
    A() { cout << "ctor A" << endl; }
    ~A() { cout << "dtor A" << endl; }
    void talk(string name) { cout << "[" << this_thread::get_id() << "] A is talking from thread " << name << endl; }
};

// global A object which threads of type B want to use
unique_ptr<A> gA;

bool initA()
{ 
    int i = 0;
    while (i++ < 7)
    {
        this_thread::sleep_for(1s);
        cout << "[" << this_thread::get_id() << "] Initializing global object gA, it takes a lot of time... (" << i << ")" << endl;
    }
    gA.reset(new A());
    cout << "[" << this_thread::get_id() << "] gA is ready to use!" << endl;
    return true;
};

void funB(string name, shared_future<bool> futA)
{
    int i = 0;
    while (i++ < 3)
    {
        this_thread::sleep_for(1s);
        cout << "[" << this_thread::get_id() << "] Initializing thread " << name << "... (" << i << ")" << endl;
    }
    cout << "[" << this_thread::get_id() << "] Now thread " << name << " waits to use global gA" << endl;
    futA.wait();

    cout << "[" << this_thread::get_id() << "] Ok, now thread " << name << " can use global gA" << endl;
    gA->talk(name);
}

int main()
{
    cout << "[" << this_thread::get_id() << "] Starting main..." << endl;

    shared_future<bool> sharedFutA(async(launch::async, initA));

    thread tB1(funB, string("B1"), sharedFutA);
    thread tB2(funB, string("B2"), sharedFutA);
    thread tB3(funB, string("B3"), sharedFutA);

    tB1.join();
    tB2.join();
    tB3.join();

    cout << "Done" << endl;
}
\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

Seems reasonable to me. Just a few nits first:

  • You should never using namespace std; in real code. Just write std::string, and so on. It'll save your readers some confusion, and it'll avoid subtle bugs related to ADL that you'll only learn about much later in life.

  • using namespace std::chrono_literals is presumably a type for using namespace std::literals or maybe using namespace std::literals::chrono_literals.

  • You've got some spurious semicolons: }; should be } in at least one place.


Unless the value of the bool being passed through the future — true or false — is meaningful, it shouldn't be part of your program. In this case you could (and therefore should) be using std::shared_future<void> instead of std::shared_future<bool>.


There is a simpler and therefore perhaps better thread-synchronization mechanism, if all your threads are launching from the same spot at the same time. Use a mutex which is locked when the threads spawn, and force each thread to "check in" by locking that mutex before it's allowed to continue. For example:

// global A object which threads of type B want to use
std::unique_ptr<A> gA;

void initA(std::unique_lock<std::mutex> lk)
{ 
    // do setup here

    gA = std::make_unique<A>();

    // now signal that it's okay for the B threads to start
    lk.unlock();
}

void funB(std::string name, std::mutex& checkpoint)
{
    // do initial setup here

    checkpoint.lock(); checkpoint.unlock();

    gA->talk(name);
}

int main() {
    std::mutex checkpoint;
    std::unique_lock<std::mutex> lk(checkpoint);
    auto tB1 = std::thread(funB, string("B1"), std::ref(checkpoint));
    auto tB2 = std::thread(funB, string("B2"), std::ref(checkpoint));
    auto tB3 = std::thread(funB, string("B3"), std::ref(checkpoint));
    initA(std::move(lk));
[...]

Notice that the lk.unlock() in initA isn't strictly needed, as written, because the destructor of lk was going to drop that lock anyway. But you could put more code after lk.unlock() if you had more things to do that didn't affect gA.


Also notice that I'm using make_unique instead of reset(new ...). Always prefer to use make_unique and make_shared instead of raw new and delete — again because it helps avoid subtle exception-related bugs which if you're lucky you'll never see in the wild; but also because if you get in the habit of never writing new or delete yourself, you'll be able to spot more easily when other people's code incorrectly uses new or delete. Ideally, you want to get in a situation where you can install a git commit hook that does a git grep \bnew\b and fails if it finds any!

\$\endgroup\$
5
  • \$\begingroup\$ Subtract a point from yourself for saying this looks reasonable. \$\endgroup\$ Aug 13, 2017 at 17:06
  • \$\begingroup\$ Well, okay, it's definitely overkill if all he needed was an A singleton, you're right. (Even then, std::call_once might be useful.) But something like his code or my rewrite would be necessary if it were important that the main thread initialize A. After all, there are some rare things (in UI programming and whatnot) that need to happen on the main thread; handling them in "whatever random thread happens to need the singleton first" might be incorrect. I agree in hindsight it is unlikely that this is OP's situation. :) \$\endgroup\$ Aug 13, 2017 at 20:25
  • \$\begingroup\$ Thank you guys for your help. I appreciate that. I have learned a lot! \$\endgroup\$
    – YotKay
    Aug 13, 2017 at 21:54
  • \$\begingroup\$ Just one more question: how would the code be changed if I didn't use the global variable? Let's imagine I have another class, which contains the A object. The big class, called BIG, initializes its differents members, including member m_A (of class A). BIG also starts some worker threads which will use m_A. What should the code look like? \$\endgroup\$
    – YotKay
    Aug 13, 2017 at 22:01
  • \$\begingroup\$ For your version with the shared_future<void> or my version with the unique_lock, there'd be no difference. You'd just do the same synchronization as before, but change what you put in the initA function/method. \$\endgroup\$ Aug 14, 2017 at 0:14
2
\$\begingroup\$

This is a terrible idea as doing initialize. The standard language features will do all this for you with zero effort.

If you have a global like this.
There are many issues. In fact the standard advice is not to have globals.

 Type    actionItem;   // Issue. When will this be initialized?

But say you do need one. You can't do it like that because of lots of other issues.

Solution: You wrap your global in a function.

 Type&   getActionItem()
 {
     static Type  actionItem;   // Notice the static
     return actionItem;
 }

You wrap your global variable in a function and make it a static member of the function. Then you can retrieve it with a call to the function. It is created on first use (so side affect it is lazily created) destroyed in the correct order and its construction is Thread Safe.

So lets look at your situation

  • I want to initialize global object of type A (named 'gA'), which may take very long time

By wrapping the global in a function we have made its construction lazy. It will be created the first time the function is called.

  • I don't want to wait for gA to be initialized, I want to start other threads at the same time (in order for the program to be responsive)

You can call the global function with any thread to start the initialization processes. There is no requirement that the object is used.

  • Other threads also take some time to initialize their own stuff, and when they are ready, they want to use the gA. It can happen at any moment, before or after gA is initialized. I must assume that other threads are ready before gA is initialized and must wait therefore wait for gA.

By wrapping it in a function and retrieving it via a function call you can guarantee this already with no special code. static storage duration object are guaranteed to be initialized in a thread safe way. The first thread to call the function will initialize the function static object. Other threads that call the function will pause while it is created.

So looking at your code.

A& getGlobalA()
{
    static A globalA;
    return globalA;
}

void funB(string const& name)  // Notice I added const&
{
    int i = 0;
    while (i++ < 3)
    {
        this_thread::sleep_for(1s);
        cout << "[" << this_thread::get_id() << "] Initializing thread " << name << "... (" << i << ")" << endl;
    }
    cout << "[" << this_thread::get_id() << "] Now thread " << name << " waits to use global gA" << endl;

    cout << "[" << this_thread::get_id() << "] Ok, now thread " << name << " can use global gA" << endl;
    getGlobalA().talk(name);
}

int main()
{
    // All this thread does is initialize your global object.
    // It is doing it in parallel to your other work. You
    // don't actually need this. If you just use your normal threads
    // then the first one to call getGlobalA() will initialize
    // the object while others automatically wait.
    thread init(getGlobalA);

    // These threads do your other work.
    // When they want to use the `globalA` they simply call
    // `getGlobalA()` if construction has not completed they
    // will pause until it is ready otherwise it will simply
    // return a reference.
    thread tB1(funB, "B1");
    thread tB2(funB, "B2");
    thread tB3(funB, "B3");


    // Note: You must wait for all threads to exit.
    //       If the thread object is destroyed before
    //       the "thread of execution" is finished then you
    //       terminate the application.
    init.join();
    tB1.join();
    tB2.join();
    tB3.join();
}
\$\endgroup\$
4
  • \$\begingroup\$ Wow, that looks great. Thank you. As a matter of fact I wasn't aware about the thread safety guarantee of static variables. Looks like I must read some doc on this. Thanks very much, your comments were very helpful! \$\endgroup\$
    – YotKay
    Aug 13, 2017 at 21:51
  • \$\begingroup\$ Just one more question: how would the code be changed if I didn't use the global variable? Let's imagine I have another class, which contains the A object. The big class, called BIG, initializes its differents members, including member m_A (of class A). BIG also starts some worker threads which will use m_A. What should the code look like? \$\endgroup\$
    – YotKay
    Aug 13, 2017 at 22:03
  • \$\begingroup\$ The equivalent of "static initialization" for a non-static member variable "m_A" would involve std::call_once(m_onceFlag, [this](){ init(m_A); });. \$\endgroup\$ Aug 14, 2017 at 0:12
  • \$\begingroup\$ Thank you. That sounds great. This is my first review here and I must say I learned a lot. I am going to use this site more frequently, as I see I can get great remarks. Thanks again! \$\endgroup\$
    – YotKay
    Aug 14, 2017 at 6:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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