Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10783 )
Change subject: Synchronizer::WaitFor thread-safety ...................................................................... Patch Set 2: (10 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? http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@41 PS2, Line 41: // Test completing the synchronizer through each of the APIs it exposes. If the hypothetical failure mode for this test is a deadlock or other test hang, consider calling alarm(30) or some such in the beginning of the test (and adding a ScopedCleanup for alarm(0) right after). You could potentially build it into the test fixture too. Below too. 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? 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 ignore_result()? Below too. 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? http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@100 PS2, Line 100: CHECK_OK ASSERT_OK? http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@111 PS2, Line 111: CHECK ASSERT 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 decoupled from that of the synchronizer itself, and that they may outlive the synchronizer if needed? 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 synchronizer), it should at least share code with Callback(). 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 do, which significantly weakens the protection offered by this DCHECK. Is it really that important to reset data_->status? Aren't we guaranteed to overwrite it any time the latch counts down? If we omitted the reset of data_->status, there'd be no race, right? -- 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: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Thu, 21 Jun 2018 20:39:42 +0000 Gerrit-HasComments: Yes
