Fix race condition: instance map wasn't locked for the

duration of the whole 'flush instances'. As a consequence,
it was possible to query instance map, while it is in the
inconsistent state. The patch was reworked after review.


server-tools/instance-manager/guardian.cc:
  do not lock instance map in Guardian_thread::init()
server-tools/instance-manager/instance_map.cc:
  Eliminate race condition: lock instance map and guardian
  for the duration of the whole "FLUSH INSTANCES" execution.
server-tools/instance-manager/instance_map.h:
  add new method. cleanup interface. add comments.
server-tools/instance-manager/manager.cc:
  use instance_map.flush_instances instead of instance_map.load() and guardian_thread.init()

server-tools/instance-manager/guardian.cc

@@ -256,7 +256,6 @@ int Guardian_thread::init()
   Instance *instance;
   Instance_map::Iterator iterator(instance_map);
 
-  instance_map->lock();
   /* clear the list of guarded instances */
   free_root(&alloc, MYF(0));
   init_alloc_root(&alloc, MEM_ROOT_BLOCK_SIZE, 0);
@@ -272,7 +271,6 @@ int Guardian_thread::init()
       }
   }
 
-  instance_map->unlock();
   return 0;
 }
 

server-tools/instance-manager/instance_map.cc

@@ -80,19 +80,44 @@ static void delete_instance(void *u)
 static int process_option(void *ctx, const char *group, const char *option)
 {
   Instance_map *map= NULL;
+
+  map = (Instance_map*) ctx;
+  return map->process_one_option(group, option);
+}
+
+C_MODE_END
+
+
+/*
+  Process one option from the configuration file.
+
+  SYNOPSIS
+    Instance_map::process_one_option()
+      group         group name
+      option        option string (e.g. "--name=value")
+
+  DESCRIPTION
+    This is an auxiliary function and should not be used externally.
+    It is used only by flush_instances(), which pass it to
+    process_option(). The caller ensures proper locking
+    of the instance map object.
+*/
+
+int Instance_map::process_one_option(const char *group, const char *option)
+{
   Instance *instance= NULL;
   static const char prefix[]= { 'm', 'y', 's', 'q', 'l', 'd' };
 
-  map = (Instance_map*) ctx;
   if (strncmp(group, prefix, sizeof prefix) == 0 &&
       ((my_isdigit(default_charset_info, group[sizeof prefix]))
        || group[sizeof(prefix)] == '\0'))
     {
-      if ((instance= map->find(group, strlen(group))) == NULL)
+      if (!(instance= (Instance *) hash_search(&hash, (byte *) group,
+                                               strlen(group))))
       {
-        if ((instance= new Instance) == 0)
+        if (!(instance= new Instance))
           goto err;
-        if (instance->init(group) || map->add_instance(instance))
+        if (instance->init(group) || my_hash_insert(&hash, (byte *) instance))
           goto err_instance;
       }
 
@@ -108,8 +133,6 @@ static int process_option(void *ctx, const char *group, const char *option)
   return 1;
 }
 
-C_MODE_END
-
 
 Instance_map::Instance_map(const char *default_mysqld_path_arg):
 mysqld_path(default_mysqld_path_arg)
@@ -144,30 +167,61 @@ void Instance_map::unlock()
   pthread_mutex_unlock(&LOCK_instance_map);
 }
 
+/*
+  Re-read instance configuration file.
+
+  SYNOPSIS
+    Instance_map::flush_instances()
+
+  DESCRIPTION
+    This function will:
+     - clear the current list of instances. This removes both
+       running and stopped instances.
+     - load a new instance configuration from the file.
+     - pass on the new map to the guardian thread: it will start
+       all instances that are marked `guarded' and not yet started.
+    Note, as the check whether an instance is started is currently
+    very simple (returns true if there is a MySQL server running
+    at the given port), this function has some peculiar
+    side-effects:
+     * if the port number of a running instance was changed, the
+       old instance is forgotten, even if it was running. The new
+       instance will be started at the new port.
+     * if the configuration was changed in a way that two
+       instances swapped their port numbers, the guardian thread
+       will not notice that and simply report that both instances
+       are configured successfully and running.
+    In order to avoid such side effects one should never call
+    FLUSH INSTANCES without prior stop of all running instances.
+
+  TODO
+    FLUSH INSTANCES should return an error if it's called
+    while there is a running instance.
+*/
 
 int Instance_map::flush_instances()
 {
   int rc;
 
+  /*
+    Guardian thread relies on the instance map repository for guarding
+    instances. This is why refreshing instance map, we need (1) to stop
+    guardian (2) reload the instance map (3) reinitialize the guardian
+    with new instances.
+  */
   guardian->lock();
   pthread_mutex_lock(&LOCK_instance_map);
   hash_free(&hash);
   hash_init(&hash, default_charset_info, START_HASH_SIZE, 0, 0,
             get_instance_key, delete_instance, 0);
-  pthread_mutex_unlock(&LOCK_instance_map);
   rc= load();
   guardian->init();
+  pthread_mutex_unlock(&LOCK_instance_map);
   guardian->unlock();
   return rc;
 }
 
 
-int Instance_map::add_instance(Instance *instance)
-{
-  return my_hash_insert(&hash, (byte *) instance);
-}
-
-
 Instance *
 Instance_map::find(const char *name, uint name_len)
 {
@@ -190,10 +244,9 @@ int Instance_map::complete_initialization()
     if ((instance= new Instance) == 0)
       goto err;
 
-    if (instance->init("mysqld") || add_instance(instance))
+    if (instance->init("mysqld") || my_hash_insert(&hash, (byte *) instance))
       goto err_instance;
 
-
     /*
       After an instance have been added to the instance_map,
       hash_free should handle it's deletion => goto err, not

server-tools/instance-manager/instance_map.h

@@ -63,21 +63,24 @@ class Instance_map
   void lock();
   void unlock();
   int init();
+  /*
+    Process a given option and assign it to appropricate instance. This is
+    required for the option handler, passed to my_search_option_files().
+  */
+  int process_one_option(const char *group, const char *option);
 
   Instance_map(const char *default_mysqld_path_arg);
   ~Instance_map();
 
-  /* loads options from config files */
-  int load();
-  /* adds instance to internal hash */
-  int add_instance(Instance *instance);
-  /* inits instances argv's after all options have been loaded */
-  int complete_initialization();
-
 public:
   const char *mysqld_path;
   Guardian_thread *guardian;
 
+private:
+  /* loads options from config files */
+  int load();
+  /* inits instances argv's after all options have been loaded */
+  int complete_initialization();
 private:
   enum { START_HASH_SIZE = 16 };
   pthread_mutex_t LOCK_instance_map;

server-tools/instance-manager/manager.cc

@@ -135,15 +135,6 @@ void manager(const Options &options)
   if (instance_map.init() || user_map.init())
     return;
 
-
-  if (instance_map.load())
-  {
-    log_error("Cannot init instances repository. This might be caused by "
-               "the wrong config file options. For instance, missing mysqld "
-               "binary. Aborting.");
-    return;
-  }
-
   if (user_map.load(options.password_file_name))
     return;
 
@@ -207,12 +198,13 @@ void manager(const Options &options)
 
   shutdown_complete= FALSE;
 
-  /* init list of guarded instances */
-  guardian_thread.lock();
-
-  guardian_thread.init();
-
-  guardian_thread.unlock();
+  if (instance_map.flush_instances())
+  {
+    log_error("Cannot init instances repository. This might be caused by "
+               "the wrong config file options. For instance, missing mysqld "
+               "binary. Aborting.");
+    return;
+  }
 
   /*
     After the list of guarded instances have been initialized,