Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18867 )
Change subject: [threadpool] Fix unsafe behaviour when SchedulerThread shutdown ...................................................................... Patch Set 13: (8 comments) http://gerrit.cloudera.org:8080/#/c/18867/13/src/kudu/util/threadpool.h File src/kudu/util/threadpool.h: http://gerrit.cloudera.org:8080/#/c/18867/13/src/kudu/util/threadpool.h@187 PS13, Line 187: struct SchedulerTask { : ThreadPoolToken* thread_pool_token; : std::function<void()> f; : }; Is this structure really needs to be public? If indeed so, then please document this structure: what is it for and how its fields are used. Can 'thread_pool_token' be null? http://gerrit.cloudera.org:8080/#/c/18867/13/src/kudu/util/threadpool.h@192 PS13, Line 192: void Schedule(ThreadPoolToken* token, : std::function<void()> f, : const MonoTime& execute_time) { Please document this public method as well: what it does and how the parameters are used. Can 'token' be null? If yes, add a test scenario where nullptr is passed for 'token'. http://gerrit.cloudera.org:8080/#/c/18867/13/src/kudu/util/threadpool.h@276 PS13, Line 276: Status Schedule(ThreadPoolToken* token, std::function<void()> f, MonoTime execute_time); Please follow the pattern for other methods in this interface and add a concise description of what this method does. Can 'token' be null? If not, then why to add Schedule() to ThreadPool API, not just ThreadPoolToken? If yes, then please add a test scenario where 'token' is passed as 'nullptr'. http://gerrit.cloudera.org:8080/#/c/18867/13/src/kudu/util/threadpool.h@506 PS13, Line 506: void WaitForScheduler(); Where is this method used in addition to the ThreadPoolTest.TestSimpleTasks test scenario? If nowhere else then why to introduce this method at all? http://gerrit.cloudera.org:8080/#/c/18867/13/src/kudu/util/threadpool.h@608 PS13, Line 608: // Protect 'scheduler_'. Since WaitForScheduler() acquires another lock along with 'scheduler_lock_', please add a comment about the right sequence of doing so to avoid possible deadlocks. http://gerrit.cloudera.org:8080/#/c/18867/13/src/kudu/util/threadpool.cc File src/kudu/util/threadpool.cc: http://gerrit.cloudera.org:8080/#/c/18867/13/src/kudu/util/threadpool.cc@678 PS13, Line 678: pending tasks finish for pending tasks to finish http://gerrit.cloudera.org:8080/#/c/18867/13/src/kudu/util/threadpool.cc@678 PS13, Line 678: , for example : // at unit tests. I'm not sure this reference to unit tests is quite relevant here. Maybe, add similar comment into the unit tests themselves? http://gerrit.cloudera.org:8080/#/c/18867/13/src/kudu/util/threadpool.cc@680 PS13, Line 680: MutexLock l(scheduler_lock_); >From what I can see in the code, ThreadPool::WaitForScheduler() isn't used >anywhere but just in a ThreadPoolTest.TestSimpleTasks. With that, how do we >protect against "deadlock" situations where first >ThreadPool::WaitForScheduler() is called and then ThreadPool::Shutdown() is >called while WaitForScheduler() still holds the scheduler_lock_ and waits on >the 'idle_cond_', while there is a task which reschedules itself again and >again? -- 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: 13 Gerrit-Owner: Yuqi Du <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Wed, 14 Sep 2022 21:27:28 +0000 Gerrit-HasComments: Yes
