You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Yuqi Du (Code Review)" <ge...@cloudera.org> on 2022/11/01 07:58:39 UTC

[kudu-CR] [threadpool] Fix unsafe behaviour when SchedulerThread shutdown

Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/18867 )

Change subject: [threadpool] Fix unsafe behaviour when SchedulerThread shutdown
......................................................................


Patch Set 17:

(4 comments)

Thanks your crs.

http://gerrit.cloudera.org:8080/#/c/18867/16//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/18867/16//COMMIT_MSG@9
PS16, Line 9: When shutting down a thread pool with Schedule
> Done
Done


http://gerrit.cloudera.org:8080/#/c/18867/16//COMMIT_MSG@9
PS16, Line 9: When shutting down a thread pool with SchedulerThread, thread pool's variable
            : 'scheduler_' is deleted and set to nullptr, at the same time, if thread pool
            : token is not shutdown and a task is scheduled on the toke
> Could you explain what scenario we need to call token->Schedule() after shu
There are 3 participants:
1. user threads, which use token->Scheduler(...), set a delayed task after a period time.
2. threadpool' threads, which will run the delay tasks or normal tasks.
3. threadpool's scheduler  thread,  step 1's delayed tasks will firstly be pushed into scheduler thread's queue, and then every period take some tasks to run.

The  safe shutdown orders as  'class SchedulerThread {' comments:
1. thread pool shutdown: to shutdown scheduler thread. (The following codes will do threadpool's shutdown)
2. token shutdown and release
3. pool shutdown(In fact 1 has shutdown) and release

At step 1, it takes a small period time. 
First scheduler_thread shutdown, then execute shutdown logic codes of thread pool before.

When scheduler_thread shutdown finished, but thread pool is not shutdown.  thread pool token can be used by user threads and token->Scheduler(...) can be called, but at the time, scheduler_thread is nullptr, so core dump would happen at https://gerrit.cloudera.org/c/18867/17/src/kudu/util/threadpool.cc#b267.


http://gerrit.cloudera.org:8080/#/c/18867/16//COMMIT_MSG@11
PS16, Line 11: uled on the tok
> Done
Done


http://gerrit.cloudera.org:8080/#/c/18867/16//COMMIT_MSG@14
PS16, Line 14: fix
> Done
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/18867
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I021422f7e51e1007c5bdbc877ab445f70ba12357
Gerrit-Change-Number: 18867
Gerrit-PatchSet: 17
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Tue, 01 Nov 2022 07:58:39 +0000
Gerrit-HasComments: Yes