From 0a03ebf8bb9cce372fb4ffa6bd6648082dbc0da3 Mon Sep 17 00:00:00 2001 From: Brian Aker Date: Thu, 22 Sep 2011 11:45:39 -0700 Subject: [PATCH] Merge in fix for unreliable test. --- libhashkit/jenkins.cc | 3 +- libmemcached/get.cc | 4 +- libmemcached/util/pool.cc | 78 +++++++++++++------ tests/mem_functions.cc | 157 ++++++++++++++++++++++++++++---------- 4 files changed, 174 insertions(+), 68 deletions(-) diff --git a/libhashkit/jenkins.cc b/libhashkit/jenkins.cc index c2001cb5..5f85ad62 100644 --- 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 21170102..3c91b495 100644 --- 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 0a3ef15f..e89789d4 100644 --- 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= ¬_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 1c5f7d4f..c6f627f7 100644 --- a/tests/mem_functions.cc +++ b/tests/mem_functions.cc @@ -52,6 +52,7 @@ #include #include #include +#include #include #include #include @@ -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 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(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}, -- 2.30.2