----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58464/#review172040 -----------------------------------------------------------
src/authentication/http/combined_authenticator.cpp Lines 291 (patched) <https://reviews.apache.org/r/58464/#comment245105> Can you add a comment here for posterity on why you take a copy instead of passing `authenticator`? Maybe reference MESOS-7065? - Vinod Kone On April 14, 2017, 11:22 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58464/ > ----------------------------------------------------------- > > (Updated April 14, 2017, 11:22 p.m.) > > > Review request for mesos, Anand Mazumdar and Vinod Kone. > > > Repository: mesos > > > Description > ------- > > This patch fixes a bug in `CombinedAuthenticator.authenticate()`. > Previously, the function registered a callback on the results of > `Authenticator::authenticate()` calls, which would be executed in > the context of the individual authenticators' processes. Since this > callback captured a copy of the `Owned<Authenticator>` referring to > that authenticator itself, it was possible that during teardown the > authenticator could be destroyed from its own context, leading to > a deadlock. > > This patch eliminates the capture of a reference to the > authenticator, instead simply capturing the authenticator's scheme. > > > Diffs > ----- > > src/authentication/http/combined_authenticator.cpp > 51ec8829cd3323f23389537d5fce4850ffcfc0e9 > > > Diff: https://reviews.apache.org/r/58464/diff/1/ > > > Testing > ------- > > `bin/mesos-tests.sh --gtest_filter="ExecutorAuthorizationTest.RunTaskGroup" > --gtest_repeat=-1 --gtest_break_on_failure` was executed both before and > after this patch to verify that it eliminates the deadlock. > > > Thanks, > > Greg Mann > >
