> On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote: > > src/authentication/cram_md5/authenticator.cpp, line 449 > > <https://reviews.apache.org/r/27760/diff/17/?file=869278#file869278line449> > > > > Shouldn't this be protected by once() to avoid 2 different threads > > loading secrets at the same time? > > Till Toenshoff wrote: > That would render our tests broken (those that reset the credentials). > The concurrency is covered by a Lock within that SASL aux plugin code. > > Till Toenshoff wrote: > Let me know if this is ok, please. > > Vinod Kone wrote: > Sorry, I don't think I follow. Can you elaborate on what specific tests > fail and why? > > Till Toenshoff wrote: > Our tests all run within the same OS-process. A ONCE-gatekeeper on > credential loading within this process would indeed allow this only a single > time. The CRAM-MD5 tests do attempt to re/load the credentials; see > cram_md5_authentication.cpp #95, #140, ... > These tests make sense given that we want to test the behavior of the > authenticator/authenticatee when using non matching credentials. > > Vinod Kone wrote: > I don't see them reloading out of band. In other words, before your patch > we used to call secrets::load() directly from tests, which I agree has thread > safety issues if secrets::load() itself is not protected. But now, with your > patch, all the loading happens via Authenticator->initialize(). So I'm trying > to understand why the protection in Authenticator initialize is not enough. > Sorry, if I'm mising something.
I added some comment that should help explaining this rather complicated issue, as we had discussed on IRC. Assuming that this covers the issue, I will now mark it as "fixed". - Till ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73310 ----------------------------------------------------------- On March 5, 2015, 7:22 p.m., Till Toenshoff wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27760/ > ----------------------------------------------------------- > > (Updated March 5, 2015, 7:22 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/Makefile.am d299f07 > src/authentication/cram_md5/authenticator.hpp c6f465f > src/authentication/cram_md5/authenticator.cpp PRE-CREATION > src/authentication/cram_md5/auxprop.hpp b894386 > src/authentication/cram_md5/auxprop.cpp cf503a2 > src/master/master.hpp 3c957ab > src/master/master.cpp 68ca19a > src/tests/cram_md5_authentication_tests.cpp 92a89c5 > > Diff: https://reviews.apache.org/r/27760/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Till Toenshoff > >
