Potential data race in ThreadGroupImpl::start() and ThreadGroupImpl::thread_run()

[ITWOI]

Hi all,

我们的bug检测工具报告了两个数据竞争，都在ThreadGroupImpl::start() 和 ThreadGroupImpl::thread_run()这两个函数中。位置分别是 thread_group_impl.c#L255, thread_group_impl.c#L135和thread_group_impl.c#L257, thread_group_impl.c#L133

下面是代码片段

    static void* thread_run(void* param)
    {
        ThreadParam* thread_param = reinterpret_cast<ThreadParam*>(param);
        // init
        if (thread_param->init_func && !thread_param->init_func->Run())
        {
            ...
            thread_param->init_fail = true;
        }
        thread_param->init_done = true;

and

    bool start()
    {
        ...
        for (int i = 0; i < _thread_num; ++i)
        {
            ....
            int ret = pthread_create(&_threads[i], NULL, &ThreadGroupImpl::thread_run, &_thread_params[i]);
            ....
                    if (_thread_params[i].init_done)
                    {
                        if (_thread_params[i].init_fail)
                        {
                            init_fail = true;
                            break;
                        }  
                    ...

这两个函数是通过pthread_create造成并发，并且上下文中没有对这两个变量的保护。

SourceBrella Inc., Yu

[qinzuoyan]

多谢 Yu。

这里因为是一个bool值的操作，所以多线程不保护实际上也没有问题。不过如果改用atomic当然会更好。

我开了一个分支 https://github.com/baidu/sofa-pbrpc/tree/issue_203 ，提交了一个修复 https://github.com/baidu/sofa-pbrpc/commit/0da6c4beffaf7f10c85d53d9b41ba4a7d94fc3c3 。

你可以拉下来，用你们的工具再检查下，这样修复是否能解决这个问题。如果问题解决了，我就把这个修复merge进来。

谢谢。 Zuoyan

[ITWOI]

感谢回复。 我重新检测了一下，没有再报告这个bug了。

[qinzuoyan]

已经merge，多谢。

[richardxx]

@qinzuoyan 这个问题不是bool已经是原子量不用写保护的问题，而是主线程对init_fail的检查可能比子线程设置它的时间早。虽然该问题已经结束，我也回复说明一下。多谢你们团队的认真工作。

[qinzuoyan]

“主线程对init_fail的检查可能比子线程设置它的时间早”——这应当是不可能的： 在thread_run()中先设置init_fail再设置init_done：

 244     static void* thread_run(void* param)
 245     {
 246         ThreadParam* thread_param = reinterpret_cast<ThreadParam*>(param);
 247         // init
 248         if (thread_param->init_func && !thread_param->init_func->Run())
 249         {
 250 #if defined( LOG )
 251             LOG(ERROR) <<  "thread_run(): init thread [" << thread_param->id << "] failed";
 252 #else
 253             SLOG(ERROR, "thread_run(): init thread [%d] failed", thread_param->id);
 254 #endif
 255             ++thread_param->init_fail;
 256         }
 257         ++thread_param->init_done;

而在主线程start()中先检查init_done后检查init_fail：

 126         // wait for init done
 127         bool init_fail = false;
 128         while (true)
 129         {
 130             int done_num = 0;
 131             for (int i = 0; i < _thread_num; ++i)
 132             {
 133                 if (_thread_params[i].init_done == 1)
 134                 {
 135                     if (_thread_params[i].init_fail == 1)
 136                     {
 137                         init_fail = true;
 138                         break;
 139                     }
 140                     else
 141                     {
 142                         ++done_num;
 143                     }
 144                 }
 145             }
 146             if (init_fail || done_num == _thread_num)
 147             {
 148                 break;                                                                                                                                                                                         
 149             }
 150             usleep(100000);
 151         }

[richardxx]

@qinzuoyan 确实如你所说，没有顺序问题。多谢。