> On March 27, 2015, 10:46 p.m., Adam B wrote: > > Fix looks good to me. Buildbot, please take another look. ;) > > Vinod Kone wrote: > Can you or @till let me know what the race and temporal issue here was? > And how this fixes it? > > Till Toenshoff wrote: > There was one major problem in these lines: > ``` > // Start authentication. > const Future<Option<string>>& future = > authenticator.get()->authenticate(from) > .onAny(defer(self(), &Self::_authenticate, pid, lambda::_1)); > ``` > > The returned future was destructed before the local reference (`future`) > would even receive it. > > On the left side, we have a const ref. On the right side, we have a > temporal (returned by `authenticate`). This in itself is fine as the lifetime > of the temporary will be extended by the lifetime of the reference. Trouble > is that the expression `onAny` then gets called on a reference to a temporary > and generates a reference to that on return. The returned reference will not > extend the lifetime of the future. Now havoc breaks lose as we have a > dangling reference. > > The problem is not showing on all systems as they OS may not immediately > reuse the destroyed objects' memory. > > For the "possible race" - that actually was not really a race - my > update-comment was misleading. It was the fact that I realized that an > onAny/... assignment could immediately trigger a callback invocation. Our > "defer" certainly delays the callback but I felt that it would look much > cleaner if I made sure that the initializing of "authenticating" happened > before assigning the callback.
Let me correct myself here; onAny gets called on a temporary, generates a reference and that is then referenced by `future`. - Till ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review78123 ----------------------------------------------------------- On March 25, 2015, 11:35 p.m., Till Toenshoff wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27760/ > ----------------------------------------------------------- > > (Updated March 25, 2015, 11:35 p.m.) > > > Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. > > > Bugs: MESOS-2050 > https://issues.apache.org/jira/browse/MESOS-2050 > > > Repository: mesos > > > Description > ------- > > The initial design and implementation of the authenticator module interface > caused issues and was not optimal for heavy lifting setup of external > dependencies. By introducing a two fold design, this has been decoupled from > the authentication message processing. The new design also gets us back on > track to the goal of makeing SASL a soft dependency of mesos. > > > Diffs > ----- > > include/mesos/authentication/authenticator.hpp f66217a > src/authentication/cram_md5/authenticator.hpp c6f465f > src/authentication/cram_md5/authenticator.cpp PRE-CREATION > src/authentication/cram_md5/auxprop.hpp b894386 > src/master/master.hpp 3c957ab > src/master/master.cpp dccd7c6 > src/tests/cram_md5_authentication_tests.cpp 92a89c5 > > Diff: https://reviews.apache.org/r/27760/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Till Toenshoff > >
