SERVER-5142 double-check in lock for new version before recreating ShardChunkManager

2 files changed, 25 insertions, 2 deletions

diff --git a/s/d_logic.h b/s/d_logic.h
index d96f937756f..4e404bad248 100644
--- a/s/d_logic.h
+++ b/s/d_logic.h
@@ -147,6 +147,10 @@ namespace mongo {
 
         // protects state below
         mutable mongo::mutex _mutex;
+        // protects accessing the config server
+        // can't use default _mutex b/c it's possible this shard is also the config server,
+        // and we need to access the ShardingState to process new connections
+        mutable mongo::mutex _configServerMutex;
 
         // map from a namespace into the ensemble of chunk ranges that are stored in this mongod
         // a ShardChunkManager carries all state we need for a collection at this shard, including its version information
diff --git a/s/d_state.cpp b/s/d_state.cpp
index 929d2a86f36..4ba1a549b29 100644
--- a/s/d_state.cpp
+++ b/s/d_state.cpp
@@ -45,7 +45,7 @@ namespace mongo {
     // -----ShardingState START ----
 
     ShardingState::ShardingState()
-        : _enabled(false) , _mutex( "ShardingState" ) {
+        : _enabled(false) , _mutex( "ShardingState" ), _configServerMutex( "_configServer_ShardingState" ) {
     }
 
     void ShardingState::enable( const string& server ) {
@@ -183,7 +183,22 @@ namespace mongo {
 
     bool ShardingState::trySetVersion( const string& ns , ConfigVersion& version /* IN-OUT */ ) {
 
-        // fast path - requested version is at the same version as this chunk manager
+        // Currently this function is called after a getVersion(), which is the first "check", and the assumption here
+        // is that we don't do anything nearly as long as a remote query in a thread between then and now.
+        // Otherwise it may be worth adding an additional check without the _configServerMutex below, since then it
+        // would be likely that the version may have changed in the meantime without waiting for or fetching config results.
+
+        // TODO:  Mutex-per-namespace?
+        
+        LOG( 2 ) << "trying to set shard version of " << version.toString() << " for '" << ns << "'" << endl;
+        
+        scoped_lock clk( _configServerMutex );
+
+        // fast path - double-check if requested version is at the same version as this chunk manager before verifying
+        // against config server
+        //
+        // This path will short-circuit the version set if another thread already managed to update the version in the
+        // meantime.  First check is from getVersion().
         //
         // cases:
         //   + this shard updated the version for a migrate's commit (FROM side)
@@ -197,6 +212,8 @@ namespace mongo {
             if ( it != _chunks.end() && it->second->getVersion() == version )
                 return true;
         }
+        
+        LOG( 2 ) << "verifying remote version against " << version.toString() << " for '" << ns << "'" << endl;
 
         // slow path - requested version is different than the current chunk manager's, if one exists, so must check for
         // newest version in the config server
@@ -209,8 +226,10 @@ namespace mongo {
         //     the secondary had no state (managers) at all, so every client request will fall here
         //   + a stale client request a version that's not current anymore
 
+        // Can't lock default mutex while creating ShardChunkManager, b/c may have to create a new connection to myself
         const string c = (_configServer == _shardHost) ? "" /* local */ : _configServer;
         ShardChunkManagerPtr p( new ShardChunkManager( c , ns , _shardName ) );
+
         {
             scoped_lock lk( _mutex );