Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/14973 )
Change subject: IMPALA-9274: cyclic barrier implementation ...................................................................... Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/14973/4/be/src/util/cyclic-barrier.h File be/src/util/cyclic-barrier.h: http://gerrit.cloudera.org:8080/#/c/14973/4/be/src/util/cyclic-barrier.h@61 PS4, Line 61: barrier_cv_.NotifyAll(); > Dropping the lock before calling notify is deliberate since the threads can Yeah, I opened a Jira for this: IMPALA-6135 (I didn't commit anything yet because I was afraid of introducing subtle synchronization bugs, but most of the time it's a quite safe optimization) There are cases when this can cause problems, e.g.: https://github.com/apache/impala/blob/master/be/src/util/promise.h#L68-L78 But the CyclicBarrier hopefully won't have this usage pattern: { CyclicBarrier b; ... // spawn other threads that call b.Wait(); b.Wait(); } destroy CyclicBarrier This would be also dangerous even if we held the mutex in Wait() for the whole time because the destructor destroys the mutex and the condition variable. Proper lifetime of the CyclicBarrier object should avoid these kind of bugs. -- To view, visit http://gerrit.cloudera.org:8080/14973 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id5c8cc6b3023d2dc03c8116d462440c5e25c3bb0 Gerrit-Change-Number: 14973 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 07 Jan 2020 11:25:40 +0000 Gerrit-HasComments: Yes
