Merge in fix for unreliable test.

diff --git a/libhashkit/jenkins.cc b/libhashkit/jenkins.cc
index c2001cb5b88f88add482532a86c584d1c78d0735..5f85ad62a220c5b04f0708c5f37776c1e2155ad7 100644 (file)
--- a/libhashkit/jenkins.cc
+++ b/libhashkit/jenkins.cc
@@ -56,11 +56,10 @@ use a bitmask.  For example, if you need only 10 bits, do
 In which case, the hash table should have hashsize(10) elements.
 */
 
-uint32_t hashkit_jenkins(const char *key, size_t length, void *context)
+uint32_t hashkit_jenkins(const char *key, size_t length, void *)
 {
   uint32_t a,b,c;                                          /* internal state */
   union { const void *ptr; size_t i; } u;     /* needed for Mac Powerbook G4 */
-  (void)context;
 
   /* Set up the internal state */
   a = b = c = 0xdeadbeef + ((uint32_t)length) + JENKINS_INITVAL;

diff --git a/libmemcached/get.cc b/libmemcached/get.cc
index 211701027bd9072024e21c3594023fa84fac79ac..3c91b495fc527f009cdb8dd5b68023d198fae5e9 100644 (file)
--- a/libmemcached/get.cc
+++ b/libmemcached/get.cc
@@ -210,7 +210,7 @@ static memcached_return_t memcached_mget_by_key_real(memcached_st *ptr,
     return rc;
   }
 
-  unlikely (ptr->flags.use_udp)
+  if (ptr->flags.use_udp)
   {
     return memcached_set_error(*ptr, MEMCACHED_NOT_SUPPORTED, MEMCACHED_AT);
   }
@@ -228,7 +228,7 @@ static memcached_return_t memcached_mget_by_key_real(memcached_st *ptr,
   }
 
   bool is_group_key_set= false;
-  if (group_key && group_key_length)
+  if (group_key and group_key_length)
   {
     if (memcached_failed(memcached_key_test(*ptr, (const char * const *)&group_key, &group_key_length, 1)))
     {

diff --git a/libmemcached/util/pool.cc b/libmemcached/util/pool.cc
index 0a3ef15f9cb76f0714f96869142a4f87758d852c..e89789d4aadccc63eba79816249f497c8700b963 100644 (file)
--- a/libmemcached/util/pool.cc
+++ b/libmemcached/util/pool.cc
@@ -164,14 +164,14 @@ static bool grow_pool(memcached_pool_st* pool)
 
 static inline memcached_pool_st *_pool_create(memcached_st* master, uint32_t initial, uint32_t max)
 {
-  if (! initial || ! max || initial > max)
+  if (initial == 0 or max == 0 or (initial > max))
   {
     errno= EINVAL;
     return NULL;
   }
 
   memcached_pool_st *object= new (std::nothrow) memcached_pool_st(master, max);
-  if (not object)
+  if (object == NULL)
   {
     errno= ENOMEM; // Set this for the failed calloc
     return NULL;
@@ -199,12 +199,13 @@ memcached_pool_st * memcached_pool(const char *option_string, size_t option_stri
 {
   memcached_st *memc= memcached(option_string, option_string_length);
 
-  if (not memc)
+  if (memc == NULL)
+  {
     return NULL;
+  }
 
-  memcached_pool_st *self;
-  self= memcached_pool_create(memc, memc->configure.initial_pool_size, memc->configure.max_pool_size);
-  if (not self)
+  memcached_pool_st *self= memcached_pool_create(memc, memc->configure.initial_pool_size, memc->configure.max_pool_size);
+  if (self == NULL)
   {
     memcached_free(memc);
     errno= ENOMEM;
@@ -219,8 +220,10 @@ memcached_pool_st * memcached_pool(const char *option_string, size_t option_stri
 
 memcached_st*  memcached_pool_destroy(memcached_pool_st* pool)
 {
-  if (not pool)
+  if (pool == NULL)
+  {
     return NULL;
+  }
 
   // Legacy that we return the original structure
   memcached_st *ret= NULL;
@@ -240,11 +243,17 @@ memcached_st* memcached_pool_pop(memcached_pool_st* pool,
                                  bool block,
                                  memcached_return_t *rc)
 {
-  assert(pool);
-  assert(rc);
-  if (not pool ||  not rc)
+  memcached_return_t not_used;
+  if (rc == NULL)
+  {
+    rc= &not_used;
+  }
+
+  if (pool == NULL)
   {
+    *rc= MEMCACHED_INVALID_ARGUMENTS;
     errno= EINVAL;
+
     return NULL;
   }
 
@@ -262,18 +271,29 @@ memcached_st* memcached_pool_pop(memcached_pool_st* pool,
     }
     else if (pool->current_size == pool->size)
     {
-      if (not block)
+      if (block == false)
       {
         *rc= mutex_exit(&pool->mutex); // this should be a different error
         return NULL;
       }
 
-      if (pthread_cond_wait(&pool->cond, &pool->mutex) == -1)
+      struct timespec time_to_wait= {0, 0};
+      time_to_wait.tv_sec = time(NULL) +5;
+      int thread_ret;
+      if ((thread_ret= pthread_cond_timedwait(&pool->cond, &pool->mutex, &time_to_wait)) != 0)
       {
-        int err= errno;
         mutex_exit(&pool->mutex);
-        errno= err;
-        *rc= MEMCACHED_ERRNO;
+        errno= thread_ret;
+
+        if (thread_ret == ETIMEDOUT)
+        {
+          *rc= MEMCACHED_TIMEOUT;
+        }
+        else
+        {
+          *rc= MEMCACHED_ERRNO;
+        }
+
         return NULL;
       }
     }
@@ -283,8 +303,7 @@ memcached_st* memcached_pool_pop(memcached_pool_st* pool,
       *rc= MEMCACHED_MEMORY_ALLOCATION_FAILURE;
       return NULL;
     }
-  }
-  while (ret == NULL);
+  } while (ret == NULL);
 
   *rc= mutex_exit(&pool->mutex);
 
@@ -293,16 +312,25 @@ memcached_st* memcached_pool_pop(memcached_pool_st* pool,
 
 memcached_return_t memcached_pool_push(memcached_pool_st* pool, memcached_st *released)
 {
-  if (not pool)
+  if (pool == NULL)
+  {
     return MEMCACHED_INVALID_ARGUMENTS;
+  }
+
+  if (released == NULL)
+  {
+    return MEMCACHED_INVALID_ARGUMENTS;
+  }
 
   memcached_return_t rc= mutex_enter(&pool->mutex);
 
   if (rc != MEMCACHED_SUCCESS)
+  {
     return rc;
+  }
 
   /* Someone updated the behavior on the object.. */
-  if (not pool->compare_version(released))
+  if (pool->compare_version(released) == false)
   {
     memcached_free(released);
     if (not (released= memcached_clone(NULL, pool->master)))
@@ -313,7 +341,7 @@ memcached_return_t memcached_pool_push(memcached_pool_st* pool, memcached_st *re
 
   pool->server_pool[++pool->firstfree]= released;
 
-  if (pool->firstfree == 0 && pool->current_size == pool->size)
+  if (pool->firstfree == 0 and pool->current_size == pool->size)
   {
     /* we might have people waiting for a connection.. wake them up :-) */
     pthread_cond_broadcast(&pool->cond);
@@ -321,7 +349,9 @@ memcached_return_t memcached_pool_push(memcached_pool_st* pool, memcached_st *re
 
   memcached_return_t rval= mutex_exit(&pool->mutex);
   if (rc == MEMCACHED_SOME_ERRORS)
+  {
     return rc;
+  }
 
   return rval;
 }
@@ -331,8 +361,10 @@ memcached_return_t memcached_pool_behavior_set(memcached_pool_st *pool,
                                                memcached_behavior_t flag,
                                                uint64_t data)
 {
-  if (not pool)
+  if (pool == NULL)
+  {
     return MEMCACHED_INVALID_ARGUMENTS;
+  }
 
   memcached_return_t rc= mutex_enter(&pool->mutex);
   if (rc != MEMCACHED_SUCCESS)
@@ -377,8 +409,10 @@ memcached_return_t memcached_pool_behavior_get(memcached_pool_st *pool,
                                                memcached_behavior_t flag,
                                                uint64_t *value)
 {
-  if (! pool)
+  if (pool == NULL)
+  {
     return MEMCACHED_INVALID_ARGUMENTS;
+  }
 
   memcached_return_t rc= mutex_enter(&pool->mutex);
   if (rc != MEMCACHED_SUCCESS)

diff --git a/tests/mem_functions.cc b/tests/mem_functions.cc
index 1c5f7d4f6bc7038c95341f0f879d2fe368b1ab1c..c6f627f7d36bb61430019339db9a21697132f829 100644 (file)
--- a/tests/mem_functions.cc
+++ b/tests/mem_functions.cc
@@ -52,6 +52,7 @@
 #include <cerrno>
 #include <memory>
 #include <pthread.h>
+#include <semaphore.h>
 #include <signal.h>
 #include <sys/stat.h>
 #include <sys/time.h>
@@ -824,7 +825,6 @@ static test_return_t bad_key_test(memcached_st *memc)
   const char *key= "foo bad";
   uint32_t flags;
   memcached_st *memc_clone;
-  size_t max_keylen= 0xffff;
 
   uint64_t query_id= memcached_query_id(memc);
   
@@ -886,8 +886,6 @@ static test_return_t bad_key_test(memcached_st *memc)
                  memcached_mget_by_key(memc_clone, "foo daddy", 9, keys, key_lengths, 1));
     test_compare(query_id +1, memcached_query_id(memc_clone));
 
-    max_keylen= 250;
-
     /* The following test should be moved to the end of this function when the
        memcached server is updated to allow max size length of the keys in the
        binary protocol
@@ -896,15 +894,16 @@ static test_return_t bad_key_test(memcached_st *memc)
                  memcached_callback_set(memc_clone, MEMCACHED_CALLBACK_NAMESPACE, NULL));
 
     std::vector <char> longkey;
-    longkey.insert(longkey.end(), max_keylen +1, 'a');
-    test_compare(longkey.size(), max_keylen +1);
+    longkey.insert(longkey.end(), MEMCACHED_MAX_KEY, 'a');
+    test_compare(longkey.size(), size_t(MEMCACHED_MAX_KEY));
     {
       size_t string_length;
-      test_null(memcached_get(memc_clone, &longkey[0], max_keylen, &string_length, &flags, &rc));
+      // We subtract 1
+      test_null(memcached_get(memc_clone, &longkey[0], longkey.size() -1, &string_length, &flags, &rc));
       test_compare(MEMCACHED_NOTFOUND, rc);
       test_zero(string_length);
 
-      test_null(memcached_get(memc_clone, &longkey[0], max_keylen +1, &string_length, &flags, &rc));
+      test_null(memcached_get(memc_clone, &longkey[0], longkey.size(), &string_length, &flags, &rc));
       test_compare(MEMCACHED_BAD_KEY_PROVIDED, rc);
       test_zero(string_length);
     }
@@ -4159,10 +4158,9 @@ static test_return_t noreply_test(memcached_st *memc)
 static test_return_t analyzer_test(memcached_st *memc)
 {
   memcached_return_t rc;
-  memcached_stat_st *memc_stat;
   memcached_analysis_st *report;
 
-  memc_stat= memcached_stat(memc, NULL, &rc);
+  memcached_stat_st *memc_stat= memcached_stat(memc, NULL, &rc);
   test_compare(MEMCACHED_SUCCESS, rc);
   test_true(memc_stat);
 
@@ -4195,25 +4193,46 @@ static test_return_t dump_test(memcached_st *memc)
 }
 
 struct test_pool_context_st {
+  volatile memcached_return_t rc;
   memcached_pool_st* pool;
   memcached_st* mmc;
+  sem_t lock;
+
+  test_pool_context_st(memcached_pool_st *pool_arg, memcached_st *memc_arg):
+    rc(MEMCACHED_FAILURE),
+    pool(pool_arg),
+    mmc(memc_arg)
+  {
+    sem_init(&lock, 0, 0);
+  }
+
+  ~test_pool_context_st()
+  {
+    sem_destroy(&lock);
+  }
 };
 
 static void* connection_release(void *arg)
 {
   test_pool_context_st *resource= static_cast<test_pool_context_st *>(arg);
+  assert(resource);
+  if (resource == NULL)
+  {
+    abort();
+  }
 
-  usleep(250);
+  sem_wait(&resource->lock);
   // Release all of the memc we are holding
-  assert(memcached_success(memcached_pool_push(resource->pool, resource->mmc)));
-  return arg;
+  resource->rc= memcached_pool_push(resource->pool, resource->mmc);
+
+  pthread_exit(arg);
 }
 
 #define POOL_SIZE 10
 static test_return_t connection_pool_test(memcached_st *memc)
 {
   memcached_pool_st* pool= memcached_pool_create(memc, 5, POOL_SIZE);
-  test_true(pool != NULL);
+  test_true(pool);
   memcached_st *mmc[POOL_SIZE];
   memcached_return_t rc;
 
@@ -4221,34 +4240,96 @@ static test_return_t connection_pool_test(memcached_st *memc)
   for (size_t x= 0; x < POOL_SIZE; ++x)
   {
     mmc[x]= memcached_pool_pop(pool, false, &rc);
-    test_true(mmc[x] != NULL);
+    test_true(mmc[x]);
     test_compare(MEMCACHED_SUCCESS, rc);
   }
 
   // All memc should be gone
-  test_true(memcached_pool_pop(pool, false, &rc) == NULL);
+  test_null(memcached_pool_pop(pool, false, &rc));
   test_compare(MEMCACHED_SUCCESS, rc);
 
-  pthread_t tid;
-  test_pool_context_st item= { pool, mmc[9] };
+  /*
+    @note This comment was written to describe what was believed to be the original authors intent.
+
+    This portion of the test creates a thread that will wait until told to free a memcached_st
+    that will be grabbed by the main thread.
+
+    It is believed that this tests whether or not we are handling ownership correctly.
+  */
+  {
+    pthread_t tid;
+    test_pool_context_st item(pool, mmc[9]);
+
+    test_zero(pthread_create(&tid, NULL, connection_release, &item));
+    mmc[9]= memcached_pool_pop(pool, true, &rc);
+    if (rc != MEMCACHED_SUCCESS)
+    {
+      sem_post(&item.lock);
+      pthread_join(tid, NULL);
+    }
+    else if (rc == MEMCACHED_TIMEOUT)
+    {
+      Error << "Possible timing issue with memcached_pool_pop";
+    }
+    else
+    {
+      test_compare(MEMCACHED_SUCCESS, rc);
+    }
+
+    sem_post(&item.lock);
+    pthread_join(tid, NULL);
+    if (rc == MEMCACHED_SUCCESS)
+    {
+      test_compare(MEMCACHED_SUCCESS, item.rc);
+      test_true(mmc[9]);
+    }
+  }
 
-  pthread_create(&tid, NULL, connection_release, &item);
-  mmc[9]= memcached_pool_pop(pool, true, &rc);
+  // Release them..
+  for (size_t x= 0; x < POOL_SIZE; ++x)
+  {
+    if (mmc[x])
+    {
+      test_compare(MEMCACHED_SUCCESS, memcached_pool_push(pool, mmc[x]));
+    }
+  }
+  test_true(memcached_pool_destroy(pool) == memc);
+
+  return TEST_SUCCESS;
+}
+
+static test_return_t connection_pool2_test(memcached_st *memc)
+{
+  memcached_pool_st* pool= memcached_pool_create(memc, 5, POOL_SIZE);
+  test_true(pool);
+  memcached_st *mmc[POOL_SIZE];
+  memcached_return_t rc;
+
+  // Fill up our array that we will store the memc that are in the pool
+  for (size_t x= 0; x < POOL_SIZE; ++x)
+  {
+    mmc[x]= memcached_pool_pop(pool, false, &rc);
+    test_true(mmc[x]);
+    test_compare(MEMCACHED_SUCCESS, rc);
+  }
+
+  // All memc should be gone
+  test_null(memcached_pool_pop(pool, false, &rc));
   test_compare(MEMCACHED_SUCCESS, rc);
-  pthread_join(tid, NULL);
-  test_true(mmc[9]);
-  const char *key= "key";
-  size_t keylen= strlen(key);
 
   // verify that I can do ops with all connections
   test_compare(MEMCACHED_SUCCESS,
-               memcached_set(mmc[0], key, keylen, "0", 1, 0, 0));
+               memcached_set(mmc[0],
+                             test_literal_param("key"),
+                             "0", 1, 0, 0));
 
   for (uint64_t x= 0; x < POOL_SIZE; ++x)
   {
     uint64_t number_value;
     test_compare(MEMCACHED_SUCCESS,
-                 memcached_increment(mmc[x], key, keylen, 1, &number_value));
+                 memcached_increment(mmc[x], 
+                                     test_literal_param("key"),
+                                     1, &number_value));
     test_compare(number_value, (x+1));
   }
 
@@ -4266,8 +4347,8 @@ static test_return_t connection_pool_test(memcached_st *memc)
   mmc[0]= memcached_pool_pop(pool, false, &rc);
   test_true(mmc[0]);
 
-  rc= memcached_pool_behavior_set(pool, MEMCACHED_BEHAVIOR_IO_MSG_WATERMARK, 9999);
-  test_compare(MEMCACHED_SUCCESS, rc);
+  test_compare(MEMCACHED_SUCCESS,
+               memcached_pool_behavior_set(pool, MEMCACHED_BEHAVIOR_IO_MSG_WATERMARK, 9999));
 
   mmc[1]= memcached_pool_pop(pool, false, &rc);
   test_true(mmc[1]);
@@ -4428,13 +4509,9 @@ static test_return_t hsieh_avaibility_test (memcached_st *memc)
 {
   test_skip(true, libhashkit_has_algorithm(HASHKIT_HASH_HSIEH));
 
-  memcached_return_t expected_rc= MEMCACHED_INVALID_ARGUMENTS;
-#ifdef HAVE_HSIEH_HASH
-  expected_rc= MEMCACHED_SUCCESS;
-#endif
-  memcached_return_t rc= memcached_behavior_set(memc, MEMCACHED_BEHAVIOR_HASH,
-                                                (uint64_t)MEMCACHED_HASH_HSIEH);
-  test_compare(expected_rc, rc);
+  test_compare(MEMCACHED_SUCCESS, 
+               memcached_behavior_set(memc, MEMCACHED_BEHAVIOR_HASH,
+                                      (uint64_t)MEMCACHED_HASH_HSIEH));
 
   return TEST_SUCCESS;
 }
@@ -4443,13 +4520,8 @@ static test_return_t murmur_avaibility_test (memcached_st *memc)
 {
   test_skip(true, libhashkit_has_algorithm(HASHKIT_HASH_MURMUR));
 
-  memcached_return_t expected_rc= MEMCACHED_INVALID_ARGUMENTS;
-#ifdef HAVE_MURMUR_HASH
-  expected_rc= MEMCACHED_SUCCESS;
-#endif
-  memcached_return_t rc= memcached_behavior_set(memc, MEMCACHED_BEHAVIOR_HASH,
-                                                (uint64_t)MEMCACHED_HASH_MURMUR);
-  test_compare(expected_rc, rc);
+  test_compare(MEMCACHED_SUCCESS,
+               memcached_behavior_set(memc, MEMCACHED_BEHAVIOR_HASH, (uint64_t)MEMCACHED_HASH_MURMUR));
 
   return TEST_SUCCESS;
 }
@@ -5836,7 +5908,8 @@ test_st tests[] ={
   {"delete_through", true, (test_callback_fn*)delete_through },
   {"noreply", true, (test_callback_fn*)noreply_test},
   {"analyzer", true, (test_callback_fn*)analyzer_test},
-  {"connectionpool", true, (test_callback_fn*)connection_pool_test },
+  {"memcached_pool_st", true, (test_callback_fn*)connection_pool_test },
+  {"memcached_pool_st #2", true, (test_callback_fn*)connection_pool2_test },
   {"memcached_pool_test", true, (test_callback_fn*)memcached_pool_test },
   {"test_get_last_disconnect", true, (test_callback_fn*)test_get_last_disconnect},
   {"verbosity", true, (test_callback_fn*)test_verbosity},