Fix data race when joining the task runner

[git-hulk]

This may close #1448

Currently, server->Join will join the task runner before the cron thread, so it may have the data race since the cron thread may access the task runner as well. We can avoid this by removing the task runner join which will do in the task runner destructor.

[torwig]

Good catch.
Sounds legitimate to me.

[git-hulk]

@torwig Thanks for your quick review.

[PragmaTwice]

Could you specify the location of another Join call of the task runner?

[git-hulk]

Could you specify the location of another Join call of the task runner?

Sure, the TaskRunner destructor will join as well: https://github.com/apache/incubator-kvrocks/blob/unstable/src/common/task_runner.h#L49

[PragmaTwice]

LGTM. Although a dtor with a long time waiting may cause some misunderstanding.

[git-hulk]

LGTM. Although a dtor with a long time waiting may cause some misunderstanding.

Yes, I think we can remove the Cancel and Join in dtor since the Server::Stop will cancel the task runner first. Can take a look again. @PragmaTwice @torwig

[torwig]

@git-hulk Yes, Server::Stop will cancel and Server::Join will join all threads.
Additionally, here https://github.com/apache/incubator-kvrocks/blob/unstable/src/common/task_runner.cc#L48 should we stop joining threads when the first call to util::ThreadJoin or just log an error and continue joining other threads?

[git-hulk]

@git-hulk Yes, Server::Stop will cancel and Server::Join will join all threads. Additionally, here https://github.com/apache/incubator-kvrocks/blob/unstable/src/common/task_runner.cc#L48 should we stop joining threads when the first call to util::ThreadJoin or just log an error and continue joining other threads?

oooh, my bad. I didn't see that, updated.

[torwig]

@git-hulk My idea was to change the loop to the following:

for (auto &thread : threads_) {
  if (auto s = util::ThreadJoin(thread); !s) {
    LOG(WARNING) << "Failed to join thread: " << s.Msg();
    continue;
  }
}

What do you think about that? @PragmaTwice What is your opinion on that?

[git-hulk]

@git-hulk My idea was to change the loop to the following:

for (auto &thread : threads_) {
  if (auto s = util::ThreadJoin(thread); !s) {
    LOG(WARNING) << "Failed to join thread: " << s.Msg();
    continue;
  }
}

What do you think about that? @PragmaTwice What is your opinion on that?

Yes, it's good to only log an error when failing to join since other places like the worker thread only print an error.

[PragmaTwice]

@git-hulk My idea was to change the loop to the following:

for (auto &thread : threads_) {
  if (auto s = util::ThreadJoin(thread); !s) {
    LOG(WARNING) << "Failed to join thread: " << s.Msg();
    continue;
  }
}

What do you think about that? @PragmaTwice What is your opinion on that?

Seems good to me.

[git-hulk]

Thanks all, merging...