Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22056 )
Change subject: [subprocess] KUDU-3624 Fix DoWait thread-safety ...................................................................... Patch Set 2: (3 comments) Whoops, it seems I forgot to post this review yesterday. http://gerrit.cloudera.org:8080/#/c/22056/2/src/kudu/util/subprocess-test.cc File src/kudu/util/subprocess-test.cc: http://gerrit.cloudera.org:8080/#/c/22056/2/src/kudu/util/subprocess-test.cc@227 PS2, Line 227: exit 0 Maybe, add an some extra delay before exiting ('sleep 1' or alike), otherwise it's not guaranteed the code below verifies what it's supposed to? http://gerrit.cloudera.org:8080/#/c/22056/2/src/kudu/util/subprocess-test.cc@233 PS2, Line 233: subprocess_thread.join(); Move this into a scoped cleanup, otherwise the thread wouldn't be ripped as necessary if the assertion above triggers. http://gerrit.cloudera.org:8080/#/c/22056/2/src/kudu/util/subprocess.cc File src/kudu/util/subprocess.cc: http://gerrit.cloudera.org:8080/#/c/22056/2/src/kudu/util/subprocess.cc@809 PS2, Line 809: std::unique_lock lock(wait_mutex_, std::try_to_lock); Since now this creates a protected section in the Subprocess's only method which might be called concurrently, maybe it's a good point to get rid of the atomics introduced in fd98e9e7331a0e8fc6b091faa2d2744a7787e6d7? Otherwise, that's quite strange to have atomics updated under a lock in the only method that supports multi-thread invocations. It might require extending the protected section to the whole scope of this Subprocess::DoWait() method, but it's not a big deal given this isn't in any sort of even remotely a 'hot' code path, and the section protection might be upgraded from shared to exclusive just before invoking wait_pid() syscall. Also, if it's required to protect the access to the 'state_' and 'child_pid_' fields elsewhere for read-only access, maybe it makes sense to switch to std::shared_mutex instead of std::mutex for wait_mutex_ and add sections protected by std::shared_lock in a few other methods. What do you think? -- To view, visit http://gerrit.cloudera.org:8080/22056 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1cb540860b439c26e1c8529123c8b29940d9f84f Gerrit-Change-Number: 22056 Gerrit-PatchSet: 2 Gerrit-Owner: Ádám Bakai <aba...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Ashwani Raina <ara...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai <aba...@cloudera.com> Gerrit-Comment-Date: Wed, 20 Nov 2024 18:06:34 +0000 Gerrit-HasComments: Yes
