From 1c437e033ddfd084c0d77d9a7ed1a062c22ab015 Mon Sep 17 00:00:00 2001 From: Brian Aker Date: Tue, 3 Apr 2012 19:07:36 -0700 Subject: [PATCH] Fix for lp:962815, from https://launchpad.net/~harebox --- libmemcached/memcached.cc | 1 + libmemcachedutil/pool.cc | 49 +++++-- libtest/test.cc | 6 +- tests/include.am | 8 +- tests/libmemcached-1.0/all_tests.h | 6 + tests/libmemcached-1.0/pool.cc | 215 ++++++++++++++++++++++++++++- tests/pool.h | 1 + 7 files changed, 264 insertions(+), 22 deletions(-) diff --git a/libmemcached/memcached.cc b/libmemcached/memcached.cc index 1726b35f..1db51e97 100644 --- a/libmemcached/memcached.cc +++ b/libmemcached/memcached.cc @@ -354,6 +354,7 @@ memcached_st *memcached_clone(memcached_st *clone, const memcached_st *source) new_clone->_namespace= memcached_array_clone(new_clone, source->_namespace); new_clone->configure.filename= memcached_array_clone(new_clone, source->_namespace); + new_clone->configure.version= source->configure.version; if (LIBMEMCACHED_WITH_SASL_SUPPORT and source->sasl.callbacks) { diff --git a/libmemcachedutil/pool.cc b/libmemcachedutil/pool.cc index a0f30c51..8816815a 100644 --- a/libmemcachedutil/pool.cc +++ b/libmemcachedutil/pool.cc @@ -241,7 +241,8 @@ memcached_st* memcached_pool_st::fetch(const struct timespec& relative_time, mem { rc= MEMCACHED_SUCCESS; - if (pthread_mutex_lock(&mutex)) + int error; + if ((error= pthread_mutex_lock(&mutex)) != 0) { rc= MEMCACHED_IN_PROGRESS; return NULL; @@ -258,7 +259,7 @@ memcached_st* memcached_pool_st::fetch(const struct timespec& relative_time, mem { if (relative_time.tv_sec == 0 and relative_time.tv_nsec == 0) { - pthread_mutex_unlock(&mutex); + error= pthread_mutex_unlock(&mutex); rc= MEMCACHED_NOTFOUND; return NULL; @@ -271,7 +272,10 @@ memcached_st* memcached_pool_st::fetch(const struct timespec& relative_time, mem int thread_ret; if ((thread_ret= pthread_cond_timedwait(&cond, &mutex, &time_to_wait)) != 0) { - pthread_mutex_unlock(&mutex); + int unlock_error; + if ((unlock_error= pthread_mutex_unlock(&mutex)) != 0) + { + } if (thread_ret == ETIMEDOUT) { @@ -288,12 +292,18 @@ memcached_st* memcached_pool_st::fetch(const struct timespec& relative_time, mem } else if (grow_pool(this) == false) { - (void)pthread_mutex_unlock(&mutex); + int unlock_error; + if ((unlock_error= pthread_mutex_unlock(&mutex)) != 0) + { + } + return NULL; } } while (ret == NULL); - pthread_mutex_unlock(&mutex); + if ((error= pthread_mutex_unlock(&mutex)) != 0) + { + } return ret; } @@ -307,7 +317,8 @@ bool memcached_pool_st::release(memcached_st *released, memcached_return_t& rc) return false; } - if (pthread_mutex_lock(&mutex)) + int error; + if ((error= pthread_mutex_lock(&mutex))) { rc= MEMCACHED_IN_PROGRESS; return false; @@ -331,10 +342,14 @@ bool memcached_pool_st::release(memcached_st *released, memcached_return_t& rc) if (firstfree == 0 and current_size == size) { /* we might have people waiting for a connection.. wake them up :-) */ - pthread_cond_broadcast(&cond); + if ((error= pthread_cond_broadcast(&cond)) != 0) + { + } } - (void)pthread_mutex_unlock(&mutex); + if ((error= pthread_mutex_unlock(&mutex)) != 0) + { + } return true; } @@ -417,7 +432,8 @@ memcached_return_t memcached_pool_behavior_set(memcached_pool_st *pool, return MEMCACHED_INVALID_ARGUMENTS; } - if (pthread_mutex_lock(&pool->mutex)) + int error; + if ((error= pthread_mutex_lock(&pool->mutex))) { return MEMCACHED_IN_PROGRESS; } @@ -426,7 +442,9 @@ memcached_return_t memcached_pool_behavior_set(memcached_pool_st *pool, memcached_return_t rc= memcached_behavior_set(pool->master, flag, data); if (memcached_failed(rc)) { - (void)pthread_mutex_unlock(&pool->mutex); + if ((error= pthread_mutex_unlock(&pool->mutex)) != 0) + { + } return rc; } @@ -455,7 +473,9 @@ memcached_return_t memcached_pool_behavior_set(memcached_pool_st *pool, } } - (void)pthread_mutex_unlock(&pool->mutex); + if ((error= pthread_mutex_unlock(&pool->mutex)) != 0) + { + } return rc; } @@ -469,14 +489,17 @@ memcached_return_t memcached_pool_behavior_get(memcached_pool_st *pool, return MEMCACHED_INVALID_ARGUMENTS; } - if (pthread_mutex_lock(&pool->mutex)) + int error; + if ((error= pthread_mutex_lock(&pool->mutex))) { return MEMCACHED_IN_PROGRESS; } *value= memcached_behavior_get(pool->master, flag); - (void)pthread_mutex_unlock(&pool->mutex); + if ((error= pthread_mutex_unlock(&pool->mutex)) != 0) + { + } return MEMCACHED_SUCCESS; } diff --git a/libtest/test.cc b/libtest/test.cc index 11222b2c..25c97c94 100644 --- a/libtest/test.cc +++ b/libtest/test.cc @@ -229,11 +229,11 @@ int main(int argc, char *argv[]) return EXIT_FAILURE; } - if (getenv("TEST_COLLECTION")) + if (getenv("YATL_COLLECTION_TO_RUN")) { - if (strlen(getenv("TEST_COLLECTION"))) + if (strlen(getenv("YATL_COLLECTION_TO_RUN"))) { - collection_to_run= getenv("TEST_COLLECTION"); + collection_to_run= getenv("YATL_COLLECTION_TO_RUN"); } } diff --git a/tests/include.am b/tests/include.am index 56e71816..96cbd70b 100644 --- a/tests/include.am +++ b/tests/include.am @@ -121,8 +121,8 @@ gdb-failure: tests/failure valgrind-cycle: tests/cycle $(VALGRIND_COMMAND) tests/cycle -valgrind-mem: tests/testapp - @$(VALGRIND_COMMAND) tests/testapp +valgrind-mem: tests/libmemcached-1.0/testapp + @$(VALGRIND_COMMAND) tests/libmemcached-1.0/testapp valgrind-failure: tests/failure @$(VALGRIND_COMMAND) tests/failure @@ -145,8 +145,8 @@ valgrind-hashplus: tests/hash_plus helgrind-cycle: tests/cycle @$(HELGRIND_COMMAND) tests/cycle -helgrind-mem: tests/testapp - @$(HELGRIND_COMMAND) tests/testapp +helgrind-mem: tests/libmemcached-1.0/testapp + @$(HELGRIND_COMMAND) tests/libmemcached-1.0/testapp helgrind-atom: tests/atomsmasher @$(HELGRIND_COMMAND) tests/atomsmasher diff --git a/tests/libmemcached-1.0/all_tests.h b/tests/libmemcached-1.0/all_tests.h index c54a5071..db82faa2 100644 --- a/tests/libmemcached-1.0/all_tests.h +++ b/tests/libmemcached-1.0/all_tests.h @@ -397,6 +397,11 @@ test_st memcached_server_add_tests[] ={ {0, 0, (test_callback_fn*)0} }; +test_st pool_TESTS[] ={ + {"lp:962815", true, (test_callback_fn*)regression_bug_962815 }, + {0, 0, (test_callback_fn*)0} +}; + test_st namespace_tests[] ={ {"basic tests", true, (test_callback_fn*)selection_of_namespace_tests }, {"increment", true, (test_callback_fn*)memcached_increment_namespace }, @@ -474,6 +479,7 @@ collection_st collection[] ={ {"touch", 0, 0, touch_tests}, {"touch", (test_callback_fn*)pre_binary, 0, touch_tests}, {"memcached_stat()", 0, 0, memcached_stat_tests}, + {"memcached_pool_create()", 0, 0, pool_TESTS}, {"kill()", 0, 0, kill_TESTS}, {0, 0, 0, 0} }; diff --git a/tests/libmemcached-1.0/pool.cc b/tests/libmemcached-1.0/pool.cc index 5b22c94e..f1d01359 100644 --- a/tests/libmemcached-1.0/pool.cc +++ b/tests/libmemcached-1.0/pool.cc @@ -47,10 +47,13 @@ using namespace libtest; #include -#include -#include +#include +#include +#include #include +#include + #ifndef __INTEL_COMPILER #pragma GCC diagnostic ignored "-Wstrict-aliasing" #endif @@ -319,3 +322,211 @@ test_return_t connection_pool3_test(memcached_st *memc) return TEST_SUCCESS; } + +static memcached_st * create_single_instance_memcached(const memcached_st *original_memc, const char *options) +{ + /* + If no options are given, copy over at least the binary flag. + */ + char options_buffer[1024]= { 0 }; + if (options == NULL) + { + if (memcached_is_binary(original_memc)) + { + snprintf(options_buffer, sizeof(options_buffer), "--BINARY"); + } + } + + /* + * I only want to hit _one_ server so I know the number of requests I'm + * sending in the pipeline. + */ + memcached_server_instance_st instance= memcached_server_instance_by_position(original_memc, 0); + + char server_string[1024]; + int server_string_length; + if (instance->type == MEMCACHED_CONNECTION_UNIX_SOCKET) + { + if (options) + { + server_string_length= snprintf(server_string, sizeof(server_string), "--SOCKET=\"%s\" %s", + memcached_server_name(instance), options); + } + else + { + server_string_length= snprintf(server_string, sizeof(server_string), "--SOCKET=\"%s\"", + memcached_server_name(instance)); + } + } + else + { + if (options) + { + server_string_length= snprintf(server_string, sizeof(server_string), "--server=%s:%d %s", + memcached_server_name(instance), int(memcached_server_port(instance)), + options); + } + else + { + server_string_length= snprintf(server_string, sizeof(server_string), "--server=%s:%d", + memcached_server_name(instance), int(memcached_server_port(instance))); + } + } + + if (server_string_length <= 0) + { + return NULL; + } + + char errror_buffer[1024]; + if (memcached_failed(libmemcached_check_configuration(server_string, server_string_length, errror_buffer, sizeof(errror_buffer)))) + { + Error << "Failed to parse (" << server_string << ") " << errror_buffer; + return NULL; + } + + return memcached(server_string, server_string_length); +} + +pthread_mutex_t mutex= PTHREAD_MUTEX_INITIALIZER; +static bool _running= false; + +static void set_running(const bool arg) +{ + int error; + if ((error= pthread_mutex_lock(&mutex)) != 0) + { + fatal_message(strerror(error)); + } + + _running= arg; + + if ((error= pthread_mutex_unlock(&mutex)) != 0) + { + fatal_message(strerror(error)); + } +} + +static bool running() +{ + int error; + bool ret; + + if ((error= pthread_mutex_lock(&mutex)) != 0) + { + fatal_message(strerror(error)); + } + + ret= _running; + + if ((error= pthread_mutex_unlock(&mutex)) != 0) + { + fatal_message(strerror(error)); + } + + return ret; +} + +static void *worker_thread(void *ctx) +{ + memcached_pool_st *pool= (memcached_pool_st *)ctx; + + while (running()) + { + memcached_return_t rc; + memcached_st *mc= memcached_pool_pop(pool, true, &rc); + + if (mc == NULL) + { + Error << "failed to fetch a connection from the pool" << memcached_strerror(NULL, rc); + dream(1, 0); + continue; + } + + rc= memcached_set(mc, "test:kv", 7, "value", 5, 600, 0); + if (memcached_failed(rc)) + { + Out << "failed memcached_set()"; + } + + rc= memcached_pool_push(pool, mc); + if (memcached_failed(rc)) + { + Error << "failed to release a connection to the pool" << memcached_strerror(NULL, rc); + } + } + + return NULL; +} + +static void sig_handler(int sig) +{ + switch (sig) + { + case SIGPROF: + set_running(false); + break; + + break; + + default: + Error << __func__ << " caught unknown signal"; + break; + } +} + +#define NUM_THREADS 20 +test_return_t regression_bug_962815(memcached_st *memc) +{ + test_skip_valgrind(); + + pthread_t pid[NUM_THREADS]; + + test_false(running()); + + memcached_st *master = create_single_instance_memcached(memc, 0); + test_true(master); + + memcached_pool_st *pool= memcached_pool_create(master, 5, 10); + + test_true(pool); + + set_running(true); + + struct itimerval new_value; + struct itimerval old_value; + memset(&new_value, 0, sizeof(itimerval)); + memset(&old_value, 0, sizeof(itimerval)); + + new_value.it_interval.tv_sec=0; + new_value.it_interval.tv_usec=10*1000; + new_value.it_value.tv_sec=0; + new_value.it_value.tv_usec=10*1000; + + test_true(signal(SIGPROF, sig_handler) != SIG_ERR); + test_compare(0, setitimer(ITIMER_PROF, &new_value, &old_value)); + + for (size_t x=0; x < NUM_THREADS; x++) + { + test_compare(0, pthread_create(&pid[x], NULL, worker_thread, (void*)pool)); + } + + for (size_t x=0; x < NUM_THREADS; x++) + { + test_compare(0, pthread_join(pid[x], NULL)); + } + + if (pool) + { + memcached_pool_destroy(pool); + } + + if (master) + { + memcached_free(master); + } + test_true(signal(SIGPROF, SIG_DFL) != SIG_ERR); + test_compare(0, setitimer(ITIMER_PROF, &old_value, NULL)); + + return TEST_SUCCESS; +} diff --git a/tests/pool.h b/tests/pool.h index 83ceaa6d..b7b29ee4 100644 --- a/tests/pool.h +++ b/tests/pool.h @@ -41,3 +41,4 @@ test_return_t memcached_pool_test(memcached_st *); test_return_t connection_pool_test(memcached_st *); test_return_t connection_pool2_test(memcached_st *); test_return_t connection_pool3_test(memcached_st *); +test_return_t regression_bug_962815(memcached_st *); -- 2.30.2