From: Trond Norbye Date: Tue, 12 May 2009 00:51:25 +0000 (+0200) Subject: Fixed up review comments from Brian X-Git-Tag: 0.29~5 X-Git-Url: https://git.m6w6.name/?a=commitdiff_plain;h=15082495242e0baf2248f255e8dac8eb286e81f2;p=m6w6%2Flibmemcached Fixed up review comments from Brian --- diff --git a/docs/libmemcachedutil.pod b/docs/libmemcachedutil.pod index d5c6b485..be410ecd 100644 --- a/docs/libmemcachedutil.pod +++ b/docs/libmemcachedutil.pod @@ -17,13 +17,12 @@ C Client Library containing utility functions for libmemcached (libmemcachedutil B is a small and thread-safe client library that provides extra functionality built on top of B. -=head1 THREADS AND PROCESSES +=head1 THREADS -When using threads or forked processes it is important to keep an instance -of C per process or thread. Without creating your own locking -structures you can not share a single C. You can though call -memcached_quit(3) on a C and then use the resulting cloned -structure. +Do not try to access an instance of C from multiple threads +at the same time. If you want to access memcached from multiple threads +you should either clone the C, or use the memcached pool +implementation. see memcached_pool_create(3). =head1 HOME diff --git a/docs/memcached_pool.pod b/docs/memcached_pool.pod index c0a818e8..5a5a9be2 100644 --- a/docs/memcached_pool.pod +++ b/docs/memcached_pool.pod @@ -13,8 +13,8 @@ C Client Library for memcached (libmemcachedutil, -lmemcachedutil) memcached_pool_st *memcached_pool_create(memcached_st* mmc, int initial, int max); memcached_st* memcached_pool_destroy(memcached_pool_st* pool); - memcached_st* memcached_pool_pop(memcached_pool_st* pool, bool block); - void memcached_pool_push(memcached_pool_st* pool, memcached_st *mmc); + memcached_st* memcached_pool_pop(memcached_pool_st* pool, bool block, memcached_return *rc); + memcached_return memcached_pool_push(memcached_pool_st* pool, memcached_st *mmc); memcached_st *memcached_create (memcached_st *ptr); @@ -40,9 +40,9 @@ passed as an argument to memcached_pool_create(), and returns the ownership of the pointer to the caller. memcached_pool_pop() is used to grab a connection structure from the -connection pool. The block argument specifies if the function should -block and wait for a connection structure to be available if we try to -exceed the maximum size. +connection pool. The block argument specifies if the function should +block and wait for a connection structure to be available if we try +to exceed the maximum size. memcached_pool_push() is used to return a connection structure back to the pool. @@ -57,7 +57,9 @@ memcached_st structure used to create the pool. If connections are in use it returns NULL. memcached_pool_pop() returns a pointer to a memcached_st structure -from the pool (or NULL if an allocation cannot be satisfied). +from the pool (or NULL if an allocation cannot be satisfied). + +memcached_pool_push() returns MEMCACHED_SUCCESS upon success. =head1 HOME diff --git a/libmemcached/memcached_pool.h b/libmemcached/memcached_pool.h index e364feb5..90e4c5ad 100644 --- a/libmemcached/memcached_pool.h +++ b/libmemcached/memcached_pool.h @@ -17,11 +17,14 @@ extern "C" { struct memcached_pool_st; typedef struct memcached_pool_st memcached_pool_st; -memcached_pool_st *memcached_pool_create(memcached_st* mmc, int initial, - int max); +memcached_pool_st *memcached_pool_create(memcached_st* mmc, uint32_t initial, + uint32_t max); memcached_st* memcached_pool_destroy(memcached_pool_st* pool); -memcached_st* memcached_pool_pop(memcached_pool_st* pool, bool block); -void memcached_pool_push(memcached_pool_st* pool, memcached_st *mmc); +memcached_st* memcached_pool_pop(memcached_pool_st* pool, + bool block, + memcached_return* rc); +memcached_return memcached_pool_push(memcached_pool_st* pool, + memcached_st* mmc); #ifdef __cplusplus } diff --git a/libmemcachedutil/memcached_pool.c b/libmemcachedutil/memcached_pool.c index 78ba40e4..766375f1 100644 --- a/libmemcachedutil/memcached_pool.c +++ b/libmemcachedutil/memcached_pool.c @@ -13,52 +13,23 @@ struct memcached_pool_st int current_size; }; -/** - * Lock a the pthread mutex and handle error conditions. If we fail to - * lock the mutex there must be something really wrong and we cannot continue. - */ -static void mutex_enter(pthread_mutex_t *mutex) +static memcached_return mutex_enter(pthread_mutex_t *mutex) { int ret; do ret= pthread_mutex_lock(mutex); while (ret == -1 && errno == EINTR); - if (ret == -1) - { - /* - ** This means something is seriously wrong (deadlock or an internal - ** error in the posix library. Print out an error message and abort - ** the program. - */ - fprintf(stderr, "pthread_mutex_lock failed: %s\n", strerror(errno)); - fflush(stderr); - abort(); - } + return (ret == -1) ? MEMCACHED_ERRNO : MEMCACHED_SUCCESS; } -/** - * Unlock a the pthread mutex and handle error conditions. - * All errors except EINTR is fatal errors and will terminate the program - * with a coredump. - */ -static void mutex_exit(pthread_mutex_t *mutex) { +static memcached_return mutex_exit(pthread_mutex_t *mutex) { int ret; do - ret = pthread_mutex_unlock(mutex); + ret= pthread_mutex_unlock(mutex); while (ret == -1 && errno == EINTR); - if (ret == -1) - { - /* - ** This means something is seriously wrong (deadlock or an internal - ** error in the posix library. Print out an error message and abort - ** the program. - */ - fprintf(stderr, "pthread_mutex_unlock %s\n", strerror(errno)); - fflush(stderr); - abort(); - } + return (ret == -1) ? MEMCACHED_ERRNO : MEMCACHED_SUCCESS; } /** @@ -66,7 +37,7 @@ static void mutex_exit(pthread_mutex_t *mutex) { * original memcached handle. */ static int grow_pool(memcached_pool_st* pool) { - memcached_st *obj = calloc(1, sizeof(*obj)); + memcached_st *obj= calloc(1, sizeof(*obj)); if (obj == NULL) return -1; @@ -82,7 +53,8 @@ static int grow_pool(memcached_pool_st* pool) { return 0; } -memcached_pool_st *memcached_pool_create(memcached_st* mmc, int initial, int max) +memcached_pool_st *memcached_pool_create(memcached_st* mmc, + uint32_t initial, uint32_t max) { memcached_pool_st* ret = NULL; memcached_pool_st object = { .mutex = PTHREAD_MUTEX_INITIALIZER, @@ -95,7 +67,7 @@ memcached_pool_st *memcached_pool_create(memcached_st* mmc, int initial, int max if (object.mmc != NULL) { - ret = calloc(1, sizeof(*ret)); + ret= calloc(1, sizeof(*ret)); if (ret == NULL) { free(object.mmc); @@ -119,11 +91,11 @@ memcached_st* memcached_pool_destroy(memcached_pool_st* pool) { memcached_st *ret = pool->master; - for (int ii = 0; ii <= pool->firstfree; ++ii) + for (int xx= 0; xx <= pool->firstfree; ++xx) { - memcached_free(pool->mmc[ii]); - free(pool->mmc[ii]); - pool->mmc[ii] = NULL; + memcached_free(pool->mmc[xx]); + free(pool->mmc[xx]); + pool->mmc[xx] = NULL; } pthread_mutex_destroy(&pool->mutex); @@ -134,53 +106,63 @@ memcached_st* memcached_pool_destroy(memcached_pool_st* pool) return ret; } -memcached_st* memcached_pool_pop(memcached_pool_st* pool, bool block) { - memcached_st *ret = NULL; - mutex_enter(&pool->mutex); +memcached_st* memcached_pool_pop(memcached_pool_st* pool, + bool block, + memcached_return *rc) +{ + memcached_st *ret= NULL; + if ((*rc= mutex_enter(&pool->mutex)) != MEMCACHED_SUCCESS) + return NULL; + do { if (pool->firstfree > -1) - ret = pool->mmc[pool->firstfree--]; + ret= pool->mmc[pool->firstfree--]; else if (pool->current_size == pool->size) { if (!block) { - mutex_exit(&pool->mutex); + *rc= mutex_exit(&pool->mutex); return NULL; } - + if (pthread_cond_wait(&pool->cond, &pool->mutex) == -1) { - /* - ** This means something is seriously wrong (an internal error in the - ** posix library. Print out an error message and abort the program. - */ - fprintf(stderr, "pthread cond_wait %s\n", strerror(errno)); - fflush(stderr); - abort(); + int err = errno; + mutex_exit(&pool->mutex); + errno = err; + *rc= MEMCACHED_ERRNO; + return NULL; } } else if (grow_pool(pool) == -1) { - mutex_exit(&pool->mutex); - return NULL; + *rc= mutex_exit(&pool->mutex); + return NULL; } } while (ret == NULL); - mutex_exit(&pool->mutex); + *rc= mutex_exit(&pool->mutex); + return ret; } -void memcached_pool_push(memcached_pool_st* pool, memcached_st *mmc) +memcached_return memcached_pool_push(memcached_pool_st* pool, + memcached_st *mmc) { - mutex_enter(&pool->mutex); - pool->mmc[++pool->firstfree] = mmc; + memcached_return rc= mutex_enter(&pool->mutex); + + if (rc != MEMCACHED_SUCCESS) + return rc; + + pool->mmc[++pool->firstfree]= mmc; if (pool->firstfree == 0 && pool->current_size == pool->size) { /* we might have people waiting for a connection.. wake them up :-) */ pthread_cond_broadcast(&pool->cond); } - mutex_exit(&pool->mutex); + + return mutex_exit(&pool->mutex); } diff --git a/tests/function.c b/tests/function.c index d7e70ac2..83702221 100644 --- a/tests/function.c +++ b/tests/function.c @@ -3383,7 +3383,7 @@ static void* connection_release(void *arg) { } *resource= arg; usleep(250); - memcached_pool_push(resource->pool, resource->mmc); + assert(memcached_pool_push(resource->pool, resource->mmc) == MEMCACHED_SUCCESS); } static test_return connection_pool_test(memcached_st *memc) @@ -3391,27 +3391,31 @@ static test_return connection_pool_test(memcached_st *memc) memcached_pool_st* pool= memcached_pool_create(memc, 5, 10); assert(pool != NULL); memcached_st* mmc[10]; - + memcached_return rc; + for (int x= 0; x < 10; ++x) { - mmc[x]= memcached_pool_pop(pool, false); + mmc[x]= memcached_pool_pop(pool, false, &rc); assert(mmc[x] != NULL); + assert(rc == MEMCACHED_SUCCESS); } - assert(memcached_pool_pop(pool, false) == NULL); + assert(memcached_pool_pop(pool, false, &rc) == NULL); + assert(rc == MEMCACHED_SUCCESS); + pthread_t tid; struct { memcached_pool_st* pool; memcached_st* mmc; } item= { .pool = pool, .mmc = mmc[9] }; pthread_create(&tid, NULL, connection_release, &item); - mmc[9]= memcached_pool_pop(pool, true); + mmc[9]= memcached_pool_pop(pool, true, &rc); + assert(rc == MEMCACHED_SUCCESS); pthread_join(tid, NULL); assert(mmc[9] == item.mmc); const char *key= "key"; size_t keylen= strlen(key); // verify that I can do ops with all connections - memcached_return rc; rc= memcached_set(mmc[0], key, keylen, "0", 1, 0, 0); assert(rc == MEMCACHED_SUCCESS); @@ -3424,7 +3428,7 @@ static test_return connection_pool_test(memcached_st *memc) // Release them.. for (int x= 0; x < 10; ++x) - memcached_pool_push(pool, mmc[x]); + assert(memcached_pool_push(pool, mmc[x]) == MEMCACHED_SUCCESS); assert(memcached_pool_destroy(pool) == memc); return TEST_SUCCESS;