From 568ec40e1818152a95d207dfd98f3dd87f4da999 Mon Sep 17 00:00:00 2001 From: cedric Date: Mon, 15 Mar 2010 14:47:51 +0000 Subject: [PATCH] * eet: Reintroduce eet cache lock fix. This version also fix a bug when file wasn't an eet file. git-svn-id: http://svn.enlightenment.org/svn/e/trunk/eet@47237 7cbeb6ba-43b4-40fd-8cce-4c39aea84d33 --- src/lib/eet_lib.c | 83 +++++++++++++++++++++++++++++++-------------------- src/tests/eet_suite.c | 64 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 32 deletions(-) diff --git a/src/lib/eet_lib.c b/src/lib/eet_lib.c index 8fed189..b6c792b 100644 --- a/src/lib/eet_lib.c +++ b/src/lib/eet_lib.c @@ -280,7 +280,7 @@ eet_test_close(int test, Eet_File *ef) if (test) { ef->delete_me_now = 1; - eet_internal_close(ef, 0); + eet_internal_close(ef, EINA_TRUE); } return test; } @@ -307,6 +307,7 @@ eet_cache_find(const char *path, Eet_File **cache, int cache_num) } /* add to end of cache */ +/* this should only be called when the cache lock is already held */ static void eet_cache_add(Eet_File *ef, Eet_File ***cache, int *cache_num, int *cache_alloc) { @@ -333,7 +334,7 @@ eet_cache_add(Eet_File *ef, Eet_File ***cache, int *cache_num, int *cache_alloc) if (del_ef) { del_ef->delete_me_now = 1; - eet_internal_close(ef, 1); + eet_internal_close(ef, EINA_TRUE); } } @@ -358,6 +359,7 @@ eet_cache_add(Eet_File *ef, Eet_File ***cache, int *cache_num, int *cache_alloc) } /* delete from cache */ +/* this should only be called when the cache lock is already held */ static void eet_cache_del(Eet_File *ef, Eet_File ***cache, int *cache_num, int *cache_alloc) { @@ -849,6 +851,7 @@ eet_clearcache(void) * We need to compute the list of eet file to close separately from the cache, * due to eet_close removing them from the cache after each call. */ + LOCK_CACHE; for (i = 0; i < eet_writers_num; i++) { if (eet_writers[i]->references <= 0) num++; @@ -887,9 +890,10 @@ eet_clearcache(void) for (i = 0; i < num; i++) { - eet_close(closelist[i]); + eet_internal_close(closelist[i], EINA_TRUE); } } + UNLOCK_CACHE; } /* FIXME: MMAP race condition in READ_WRITE_MODE */ @@ -1281,6 +1285,12 @@ eet_internal_read1(Eet_File *ef) } #endif +/* + * this should only be called when the cache lock is already held + * (We could drop this restriction if we add a parameter to eet_test_close + * that indicates if the lock is held or not. For now it is easiest + * to just require that it is always held.) + */ static Eet_File * eet_internal_read(Eet_File *ef) { @@ -1302,7 +1312,7 @@ eet_internal_read(Eet_File *ef) return eet_internal_read2(ef); default: ef->delete_me_now = 1; - eet_close(ef); + eet_internal_close(ef, EINA_TRUE); break; } @@ -1317,10 +1327,13 @@ eet_internal_close(Eet_File *ef, Eina_Bool locked) /* check to see its' an eet file pointer */ if (eet_check_pointer(ef)) return EET_ERROR_BAD_OBJECT; + + if (!locked) LOCK_CACHE; + /* deref */ ef->references--; /* if its still referenced - dont go any further */ - if (ef->references > 0) return EET_ERROR_NONE; + if (ef->references > 0) goto on_error; /* flush any writes */ err = eet_flush2(ef); @@ -1329,21 +1342,16 @@ eet_internal_close(Eet_File *ef, Eina_Bool locked) /* if not urgent to delete it - dont free it - leave it in cache */ if ((!ef->delete_me_now) && (ef->mode == EET_FILE_MODE_READ)) - return EET_ERROR_NONE; + goto on_error; /* remove from cache */ - if (!locked) - { - LOCK_CACHE; - } if (ef->mode == EET_FILE_MODE_READ) eet_cache_del(ef, &eet_readers, &eet_readers_num, &eet_readers_alloc); else if ((ef->mode == EET_FILE_MODE_WRITE) || (ef->mode == EET_FILE_MODE_READ_WRITE)) eet_cache_del(ef, &eet_writers, &eet_writers_num, &eet_writers_alloc); - if (!locked) - { - UNLOCK_CACHE; - } + + /* we can unlock the cache now */ + if (!locked) UNLOCK_CACHE; DESTROY_FILE(ef); @@ -1394,6 +1402,10 @@ eet_internal_close(Eet_File *ef, Eina_Bool locked) /* free it */ free(ef); return err; + + on_error: + if (!locked) UNLOCK_CACHE; + return EET_ERROR_NONE; } EAPI Eet_File * @@ -1425,7 +1437,11 @@ eet_memopen_read(const void *data, size_t size) ef->sha1 = NULL; ef->sha1_length = 0; - return eet_internal_read(ef); + /* eet_internal_read expects the cache lock to be held when it is called */ + LOCK_CACHE; + ef = eet_internal_read(ef); + UNLOCK_CACHE; + return ef; } EAPI Eet_File * @@ -1450,7 +1466,7 @@ eet_open(const char *file, Eet_File_Mode mode) eet_flush2(ef); ef->references++; ef->delete_me_now = 1; - eet_internal_close(ef, 1); + eet_internal_close(ef, EINA_TRUE); } ef = eet_cache_find((char *)file, eet_readers, eet_readers_num); } @@ -1462,11 +1478,10 @@ eet_open(const char *file, Eet_File_Mode mode) { ef->delete_me_now = 1; ef->references++; - eet_internal_close(ef, 1); + eet_internal_close(ef, EINA_TRUE); } ef = eet_cache_find((char *)file, eet_writers, eet_writers_num); } - UNLOCK_CACHE; /* try open the file based on mode */ if ((mode == EET_FILE_MODE_READ) || (mode == EET_FILE_MODE_READ_WRITE)) @@ -1475,23 +1490,23 @@ eet_open(const char *file, Eet_File_Mode mode) file_stat.st_mtime = 0; fp = fopen(file, "rb"); - if (!fp) goto on_error; + if (!fp) goto open_error; if (fstat(fileno(fp), &file_stat)) { fclose(fp); fp = NULL; - goto on_error; + goto open_error; } if ((mode == EET_FILE_MODE_READ) && (file_stat.st_size < ((int) sizeof(int) * 3))) { fclose(fp); fp = NULL; - goto on_error; + goto open_error; } - on_error: - if (fp == NULL && mode == EET_FILE_MODE_READ) return NULL; + open_error: + if (fp == NULL && mode == EET_FILE_MODE_READ) goto on_error; } else { @@ -1503,7 +1518,7 @@ eet_open(const char *file, Eet_File_Mode mode) unlink(file); fd = open(file, O_CREAT | O_TRUNC | O_RDWR, S_IRUSR | S_IWUSR); fp = fdopen(fd, "wb"); - if (!fp) return NULL; + if (!fp) goto on_error; } /* We found one */ @@ -1511,7 +1526,7 @@ eet_open(const char *file, Eet_File_Mode mode) { ef->delete_me_now = 1; ef->references++; - eet_internal_close(ef, 0); + eet_internal_close(ef, EINA_FALSE); ef = NULL; } @@ -1520,6 +1535,7 @@ eet_open(const char *file, Eet_File_Mode mode) /* reference it up and return it */ if (fp != NULL) fclose(fp); ef->references++; + UNLOCK_CACHE; return ef; } @@ -1528,7 +1544,7 @@ eet_open(const char *file, Eet_File_Mode mode) /* Allocate struct for eet file and have it zero'd out */ ef = malloc(sizeof(Eet_File) + file_len); if (!ef) - return NULL; + goto on_error; /* fill some of the members */ INIT_FILE(ef); @@ -1557,7 +1573,7 @@ eet_open(const char *file, Eet_File_Mode mode) /* if we can't open - bail out */ if (eet_test_close(!ef->fp, ef)) - return NULL; + goto on_error; fcntl(fileno(ef->fp), F_SETFD, FD_CLOEXEC); /* if we opened for read or read-write */ @@ -1567,10 +1583,10 @@ eet_open(const char *file, Eet_File_Mode mode) ef->data = mmap(NULL, ef->data_size, PROT_READ, MAP_SHARED, fileno(ef->fp), 0); if (eet_test_close((ef->data == MAP_FAILED), ef)) - return NULL; + goto on_error; ef = eet_internal_read(ef); if (!ef) - return NULL; + goto on_error; } empty_file: @@ -1584,16 +1600,19 @@ eet_open(const char *file, Eet_File_Mode mode) /* add to cache */ if (ef->references == 1) { - LOCK_CACHE; if (ef->mode == EET_FILE_MODE_READ) eet_cache_add(ef, &eet_readers, &eet_readers_num, &eet_readers_alloc); else if ((ef->mode == EET_FILE_MODE_WRITE) || (ef->mode == EET_FILE_MODE_READ_WRITE)) eet_cache_add(ef, &eet_writers, &eet_writers_num, &eet_writers_alloc); - UNLOCK_CACHE; } + UNLOCK_CACHE; return ef; + +on_error: + UNLOCK_CACHE; + return NULL; } EAPI Eet_File_Mode @@ -1654,7 +1673,7 @@ eet_identity_set(Eet_File *ef, Eet_Key *key) EAPI Eet_Error eet_close(Eet_File *ef) { - return eet_internal_close(ef, 0); + return eet_internal_close(ef, EINA_FALSE); } EAPI void * diff --git a/src/tests/eet_suite.c b/src/tests/eet_suite.c index a41e8f7..0faa01b 100644 --- a/src/tests/eet_suite.c +++ b/src/tests/eet_suite.c @@ -6,6 +6,7 @@ #include #include #include +#include #include @@ -1410,6 +1411,65 @@ START_TEST(eet_cipher_decipher_simple) } END_TEST +static Eina_Bool open_worker_stop; +static void* +open_close_worker(void* path) +{ + while (!open_worker_stop) + { + Eet_File* ef = eet_open((char const*)path, EET_FILE_MODE_READ); + if (ef == NULL) + { + pthread_exit("eet_open() failed"); + } + else + { + Eet_Error err_code = eet_close(ef); + if (err_code != EET_ERROR_NONE) + pthread_exit("eet_close() failed"); + } + } + + pthread_exit(NULL); +} + +START_TEST(eet_cache_concurrency) +{ + char *file = strdup("/tmp/eet_suite_testXXXXXX"); + const char *buffer = "test data"; + Eet_File *ef; + void *thread_ret; + unsigned int n; + + eet_init(); + + /* create a file to test with */ + fail_if(!mktemp(file)); + ef = eet_open(file, EET_FILE_MODE_WRITE); + fail_if(!ef); + fail_if(!eet_write(ef, "keys/tests", buffer, strlen(buffer) + 1, 0)); + + /* start a thread that repeatedly opens and closes a file */ + open_worker_stop = 0; + pthread_t thread; + pthread_create(&thread, NULL, open_close_worker, file); + + /* clear the cache repeatedly in this thread */ + for (n = 0; n < 100000; ++n) + { + eet_clearcache(); + } + + /* join the other thread, and fail if it returned an error message */ + open_worker_stop = 1; + fail_if(pthread_join(thread, &thread_ret) != 0); + fail_unless(thread_ret == NULL, (char const*)thread_ret); + + fail_if(unlink(file) != 0); + eet_shutdown(); +} +END_TEST + Suite * eet_suite(void) @@ -1455,6 +1515,10 @@ eet_suite(void) suite_add_tcase(s, tc); #endif + tc = tcase_create("Eet Cache"); + tcase_add_test(tc, eet_cache_concurrency); + suite_add_tcase(s, tc); + return s; } -- 2.7.4