Condition destroy error shouldn't be a fatal error

[vagetablechicken]

When we stop one be, it always makes a fatal error:

F0213 12:33:53.604131 117681 utils.cpp:1124] fail to destroy cond. err=Device or resource busy

https://github.com/apache/incubator-doris/blob/fd492e3b6fd729e617536842ba4092911f8afae8/be/src/olap/utils.cpp#L133-L139

We all know that EBUSY means destroy the object referenced by cond while it is referenced by another thread.

It's a common fault in multi-threads, so we shouldn't make it fatal after one try.
How about make it fatal after several failure attempts? As follows.

#define PTHREAD_COND_DESTROY_WITH_LOG(condptr) \
    do {\
        int cond_ret = 0;\
        int try_time = 0;\
        while (0 != (cond_ret = pthread_cond_destroy(condptr))) {\
            if (try_time++ < 20) sleep(1); \
            else LOG(FATAL) << "fail to destroy cond. err=" << strerror(cond_ret); \
        }\
    } while (0)

My test result:
It will wait 10~15s when the be is idle.
----2020/02/26----
It's my misunderstanding of wait 10s. My stop ope is:

1. send SIGTERM
2. wait 10s, if process can't exit, send SIGKILL

So, if only SIGTERM sent, BE may take longer to destory itself.
The root cause is the thread pool management. So this issue should be closed.

[imay]

My concern is that even if this is done, it may not solve the problem. I think we need to ensure that all references should be completed before the object is released. Otherwise, the destruction is successful here, and another thread may refer to the object that has already been destroyed, which may cause crash

[vagetablechicken]

Yes, this method may not help. But it can avoid coredump when we restart be. Receiving too much coredump alert sms is too painful.
😂
OK, let's consider the more perfect method.

Stack trace is

Check failure stack trace: ***
@ 0x22f3e5d google::LogMessage::Fail()
@ 0x22f5ce4 google::LogMessage::SendToLog()
@ 0x22f3984 google::LogMessage::Flush()
@ 0x22f6719 google::LogMessageFatal::~LogMessageFatal()
@ 0xe9f09f doris::Condition::~Condition()
@ 0x13a2d4d doris::TaskWorkerPool::~TaskWorkerPool()
@ 0x13a2f71 doris::TaskWorkerPool::~TaskWorkerPool()
@ 0x13a0b9b doris::AgentServer::~AgentServer()

The thread created in TaskWorkerPool will be detached.
https://github.com/apache/incubator-doris/blob/625411bd280270e1aa026a03632f3f7f6140653d/be/src/agent/task_worker_pool.cpp#L261

And because of that, when the Condition is destroying, the work threads may still running.
If we want to ensure that all references should be completed before the object is released, we only have two choices:

1. make threads joinable, waiting all threads finished in ~TaskWorkerPool() (p.s. it's similar with "wait in PTHREAD_COND_DESTROY_WITH_LOG" method. “wait in PTHREAD_COND_DESTROY_WITH_LOG” has a certain wait time, perhaps better.)
2. interrupt the running threads in ~TaskWorkerPool()

[chaoyli]

Thread in this place is not elegant and will be refactored will imported ThreadPool framework.
#2915
If you have interest, we can discuss to do it.

[vagetablechicken]

Thread in this place is not elegant and will be refactored will imported ThreadPool framework.
#2915
If you have interest, we can discuss to do it.

That's nice. Actually, I'm meant to create a issue about thread pool, which using KUDU as a reference model.
If TaskWorkerPool was replaced by KUDU::ThreadPool, it can be shutdown. And the variable number of threads is an excellent design.