Commit 21aa5867 authored by unknown's avatar unknown
Browse files

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()
parent 4e69c153

server-tools/instance-manager/guardian.cc

+0 −2
Original line number Diff line number Diff line
@@ -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

+68 −15
Original line number Diff line number Diff line
@@ -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

+10 −7
Original line number Diff line number Diff line
@@ -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

+7 −15
Original line number Diff line number Diff line
@@ -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,