You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "git-hulk (via GitHub)" <gi...@apache.org> on 2023/06/11 12:08:06 UTC
[GitHub] [incubator-kvrocks] git-hulk opened a new pull request, #1493: Fix duplicately join the task runner
git-hulk opened a new pull request, #1493:
URL: https://github.com/apache/incubator-kvrocks/pull/1493
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1493: Fix duplicately join the task runner
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1493:
URL: https://github.com/apache/incubator-kvrocks/pull/1493#discussion_r1227502722
##########
src/common/task_runner.h:
##########
@@ -42,13 +42,7 @@ class TaskRunner {
TaskRunner(const TaskRunner&) = delete;
TaskRunner& operator=(const TaskRunner&) = delete;
-
- ~TaskRunner() {
- if (state_ != Stopped) {
- Cancel();
- auto _ = Join();
- }
- }
+ ~TaskRunner() = default;
Review Comment:
The data race may be caused by the task runner's `Join` function will clear the concurrent queue, so it could be fixed after moving to the last one to join. It's good for me to keep the Cancel and Join in dctor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #1493: Fix duplicately join the task runner
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #1493:
URL: https://github.com/apache/incubator-kvrocks/pull/1493#issuecomment-1588467063
> @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1493: Fix duplicately join the task runner
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1493:
URL: https://github.com/apache/incubator-kvrocks/pull/1493#issuecomment-1587026639
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1493: Fix duplicately join the task runner
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1493:
URL: https://github.com/apache/incubator-kvrocks/pull/1493#issuecomment-1588433616
> @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 prints an error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1493: Fix duplicately join the task runner
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1493:
URL: https://github.com/apache/incubator-kvrocks/pull/1493#issuecomment-1587462592
> @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1493: Fix duplicately join the task runner
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1493:
URL: https://github.com/apache/incubator-kvrocks/pull/1493#issuecomment-1588433034
> LOG(WARNING) << "Failed to join thread: " << s.Msg();
Yes, it's good to only log an error when failing to join since other places like the worker thread only prints an error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on pull request #1493: Fix duplicately join the task runner
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on PR #1493:
URL: https://github.com/apache/incubator-kvrocks/pull/1493#issuecomment-1587323131
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1493: Fix duplicately join the task runner
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1493:
URL: https://github.com/apache/incubator-kvrocks/pull/1493#discussion_r1227489897
##########
src/common/task_runner.h:
##########
@@ -42,13 +42,7 @@ class TaskRunner {
TaskRunner(const TaskRunner&) = delete;
TaskRunner& operator=(const TaskRunner&) = delete;
-
- ~TaskRunner() {
- if (state_ != Stopped) {
- Cancel();
- auto _ = Join();
- }
- }
+ ~TaskRunner() = default;
Review Comment:
Seems we need some guarantee for this object? i.e. the behavior will be **undefined** while the taskrunner is destructed and the attached threads are still running, which is dangerous.
Can refer to std::jthread (https://en.cppreference.com/w/cpp/thread/jthread/~jthread) and std::thread (https://en.cppreference.com/w/cpp/thread/thread/~thread).
The previous dtor build such a guarantee like std::jthread, but I think we can call `Join` explicitly as far as possible to prevent misunderstanding. Removing such dtor cannot help the situation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1493: Fix duplicately join the task runner
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1493:
URL: https://github.com/apache/incubator-kvrocks/pull/1493#discussion_r1227489897
##########
src/common/task_runner.h:
##########
@@ -42,13 +42,7 @@ class TaskRunner {
TaskRunner(const TaskRunner&) = delete;
TaskRunner& operator=(const TaskRunner&) = delete;
-
- ~TaskRunner() {
- if (state_ != Stopped) {
- Cancel();
- auto _ = Join();
- }
- }
+ ~TaskRunner() = default;
Review Comment:
Seems we need some guarantee for this object? i.e. the behavior will be undefined while the taskrunner is destructed and the attached threads are still running.
Can refer to std::jthread (https://en.cppreference.com/w/cpp/thread/jthread/~jthread) and std::thread (https://en.cppreference.com/w/cpp/thread/thread/~thread).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1493: Fix duplicately join the task runner
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1493:
URL: https://github.com/apache/incubator-kvrocks/pull/1493#issuecomment-1586139898
@torwig Thanks for your quick review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on pull request #1493: Fix duplicately join the task runner
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on PR #1493:
URL: https://github.com/apache/incubator-kvrocks/pull/1493#issuecomment-1587888982
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1493: Fix duplicately join the task runner
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1493:
URL: https://github.com/apache/incubator-kvrocks/pull/1493#issuecomment-1586185895
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #1493: Fix duplicately join the task runner
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #1493:
URL: https://github.com/apache/incubator-kvrocks/pull/1493#issuecomment-1586174910
Could you specify the location of another Join call of the task runner?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1493: Fix duplicately join the task runner
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1493:
URL: https://github.com/apache/incubator-kvrocks/pull/1493#discussion_r1227489897
##########
src/common/task_runner.h:
##########
@@ -42,13 +42,7 @@ class TaskRunner {
TaskRunner(const TaskRunner&) = delete;
TaskRunner& operator=(const TaskRunner&) = delete;
-
- ~TaskRunner() {
- if (state_ != Stopped) {
- Cancel();
- auto _ = Join();
- }
- }
+ ~TaskRunner() = default;
Review Comment:
Seems we need some guarantee for this object? i.e. the behavior will be undefined while the taskrunner is destructed and the attached threads are still running.
Can refer to std::jthread (https://en.cppreference.com/w/cpp/thread/jthread/~jthread) and std::thread (https://en.cppreference.com/w/cpp/thread/thread/~thread).
The previous dtor build such a guarantee like std::jthread, but I think we can call `Join` explicitly as far as possible to prevent misunderstanding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk merged pull request #1493: Fix data race when joining the task runner
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk merged PR #1493:
URL: https://github.com/apache/incubator-kvrocks/pull/1493
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1493: Fix duplicately join the task runner
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1493:
URL: https://github.com/apache/incubator-kvrocks/pull/1493#discussion_r1226769983
##########
src/server/server.cc:
##########
@@ -230,15 +230,15 @@ void Server::Join() {
worker->Join();
}
- if (auto s = task_runner_.Join(); !s) {
- LOG(WARNING) << s.Msg();
- }
if (auto s = util::ThreadJoin(cron_thread_); !s) {
LOG(WARNING) << "Cron thread operation failed: " << s.Msg();
}
if (auto s = util::ThreadJoin(compaction_checker_thread_); !s) {
LOG(WARNING) << "Compaction checker thread operation failed: " << s.Msg();
}
+ if (auto s = task_runner_.Join(); !s) {
+ LOG(WARNING) << "Task runner thread operation failed: " << s.Msg();
Review Comment:
```suggestion
LOG(WARNING) << s.Msg();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1493: Fix data race when joining the task runner
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1493:
URL: https://github.com/apache/incubator-kvrocks/pull/1493#issuecomment-1588790746
Thanks all, merging...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1493: Fix duplicately join the task runner
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1493:
URL: https://github.com/apache/incubator-kvrocks/pull/1493#discussion_r1227489897
##########
src/common/task_runner.h:
##########
@@ -42,13 +42,7 @@ class TaskRunner {
TaskRunner(const TaskRunner&) = delete;
TaskRunner& operator=(const TaskRunner&) = delete;
-
- ~TaskRunner() {
- if (state_ != Stopped) {
- Cancel();
- auto _ = Join();
- }
- }
+ ~TaskRunner() = default;
Review Comment:
Seems we need some guarantee for this object? i.e. the behavior will be undefined while the taskrunner is destructed and the attached threads are still running.
Can refer to std::jthread (https://en.cppreference.com/w/cpp/thread/jthread/~jthread) and std::thread (https://en.cppreference.com/w/cpp/thread/thread/~thread).
The previous dtor build such a guarantee like std::jthread, but I think we can call `Join` explicitly as far as possible to prevent misunderstanding. Removing such dtor cannot help the situation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org