Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10783 )
Change subject: Synchronizer::WaitFor thread-safety ...................................................................... Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc File src/kudu/util/async_util-test.cc: http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@38 PS2, Line 38: public: > Not needed? Done http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@46 PS2, Line 46: auto waiter = thread([sync] { > Aren't these captures by copy? Isn't that disallowed? How does this work? Correct. I removed the DISALLOW_COPY_AND_ASSIGN call on Synchronizer since it's no longer necessary for correctness. This test uses the API somewhat unusually in that it waits on the synchronizer from the background thread and completes the callback on the original thread, but I don't think that's really a problem. The more conventional orientation is tested in TestSynchronizerTimedWait. http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@47 PS2, Line 47: auto s = sync.Wait(); > You're not using 's'; could you just omit the LHS? Or wrap the call in igno Done http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@96 PS2, Line 96: waiter = thread([cb] { > Capture by ref here and below? That would be a use after free in the second case, since the callback would be cleaned up before the sleep is complete. In the first case I think it's more typical for the callback to be moved into the background thread. http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@100 PS2, Line 100: CHECK_OK > ASSERT_OK? Done http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@111 PS2, Line 111: CHECK > ASSERT Done http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util.h File src/kudu/util/async_util.h: http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util.h@33 PS2, Line 33: // Simple class which can be used to make async methods synchronous. > Can you update this to describe how the lifecycle of the callbacks is decou Done http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util.h@44 PS2, Line 44: void StatusCB(const Status& status) { > No longer used? If it is used (i.e. for non-callback invocations on the syn It's used in a test, and it's part of the public API and seems reasonably useful so I didn't want to remove it. Having them share an implementation would be pretty awkward: void StatusCB(const Status& status) { Data::Callback(std::weak_ptr(data_), status); } But if that seems better to you then I can make the change. http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util.h@71 PS2, Line 71: DCHECK_EQ(data_.use_count(), 1); > Are you sure that weak refs count towards use_count()? I don't think they d Yeah you're right. I changed this initially because it smelled like a Java-style memory leak where a field is failed to be reset to null, but in practice it won't matter as you point out. -- To view, visit http://gerrit.cloudera.org:8080/10783 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a Gerrit-Change-Number: 10783 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Fri, 22 Jun 2018 19:27:38 +0000 Gerrit-HasComments: Yes
