git commit: Fixed race condition that caused Authenticatee process to deadlock.

[vi...@apache.org]

Updated Branches:
  refs/heads/master 09df8039a -> 8556d4ce9


Fixed race condition that caused Authenticatee process to deadlock.

Review: https://reviews.apache.org/r/15138


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/8556d4ce
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/8556d4ce
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/8556d4ce

Branch: refs/heads/master
Commit: 8556d4ce97653b1e4cf3d0f02323abd556f5b912
Parents: 09df803
Author: Vinod Kone <vi...@twitter.com>
Authored: Thu Oct 31 10:43:36 2013 -0700
Committer: Vinod Kone <vi...@twitter.com>
Committed: Thu Oct 31 16:59:20 2013 -0700

----------------------------------------------------------------------
 src/sched/sched.cpp | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/8556d4ce/src/sched/sched.cpp
----------------------------------------------------------------------
diff --git a/src/sched/sched.cpp b/src/sched/sched.cpp
index 3049096..0422068 100644
--- a/src/sched/sched.cpp
+++ b/src/sched/sched.cpp
@@ -109,12 +109,16 @@ public:
       // TODO(benh): Add Try().
       detector(Error("uninitialized")),
       credential(_credential),
+      authenticatee(NULL),
       authenticating(None()),
       authenticated(false),
       reauthenticate(false)
   {}
 
-  virtual ~SchedulerProcess() {}
+  virtual ~SchedulerProcess()
+  {
+    delete authenticatee;
+  }
 
 protected:
   virtual void initialize()
@@ -281,11 +285,25 @@ protected:
     LOG(INFO) << "Authenticating with master " << master;
 
     CHECK_SOME(credential);
-    Owned<sasl::Authenticatee> authenticatee =
-      new sasl::Authenticatee(credential.get(), self());
 
+    CHECK(authenticatee == NULL);
+    authenticatee = new sasl::Authenticatee(credential.get(), self());
+
+    // NOTE: We do not pass 'Owned<Authenticatee>' here because doing
+    // so could make 'AuthenticateeProcess' responsible for deleting
+    // 'Authenticatee' causing a deadlock because the destructor of
+    // 'Authenticatee' waits on 'AuthenticateeProcess'.
+    // This will happen in the following scenario:
+    // --> 'AuthenticateeProcess' does a 'Future.set()'.
+    // --> '_authenticate()' is dispatched to this process.
+    // --> This process executes '_authenticatee()'.
+    // --> 'AuthenticateeProcess' removes the onAny callback
+    //     from its queue which holds the last reference to
+    //     'Authenticatee'.
+    // --> '~Authenticatee()' is invoked by 'AuthenticateeProcess'.
+    // TODO(vinod): Consider using 'Shared' to 'Owned' upgrade.
     authenticating = authenticatee->authenticate(master)
-      .onAny(defer(self(), &Self::_authenticate, authenticatee));
+      .onAny(defer(self(), &Self::_authenticate));
 
     delay(Seconds(5),
           self(),
@@ -293,13 +311,16 @@ protected:
           authenticating.get());
   }
 
-  void _authenticate(const Owned<sasl::Authenticatee>& authenticatee)
+  void _authenticate()
   {
     if (aborted) {
       VLOG(1) << "Ignoring _authenticate because the driver is aborted!";
       return;
     }
 
+    delete CHECK_NOTNULL(authenticatee);
+    authenticatee = NULL;
+
     CHECK_SOME(authenticating);
     const Future<bool>& future = authenticating.get();
 
@@ -896,6 +917,8 @@ private:
 
   const Option<Credential> credential;
 
+  sasl::Authenticatee* authenticatee;
+
   // Indicates if an authentication attempt is in progress.
   Option<Future<bool> > authenticating;