From cfd192b0140f004404a208cde967ac8eef16cdff Mon Sep 17 00:00:00 2001 From: Ben Straub Date: Thu, 3 Oct 2013 12:44:34 -0700 Subject: [PATCH 1/4] Add test for multiple thread init/shutdown --- tests-clar/threads/basic.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests-clar/threads/basic.c b/tests-clar/threads/basic.c index a15c53140..a329ee7f9 100644 --- a/tests-clar/threads/basic.c +++ b/tests-clar/threads/basic.c @@ -21,3 +21,16 @@ void test_threads_basic__cache(void) // run several threads polling the cache at the same time cl_assert(1 == 1); } + +void test_threads_basic__multiple_init(void) +{ + git_repository *nested_repo; + + git_threads_init(); + cl_git_pass(git_repository_open(&nested_repo, cl_fixture("testrepo.git"))); + git_repository_free(nested_repo); + + git_threads_shutdown(); + cl_git_pass(git_repository_open(&nested_repo, cl_fixture("testrepo.git"))); + git_repository_free(nested_repo); +} From e411b74ebdb0ec528dbd0296746bd8a573f4045d Mon Sep 17 00:00:00 2001 From: Ben Straub Date: Fri, 4 Oct 2013 19:33:48 -0700 Subject: [PATCH 2/4] Posix synchronized init, prototype win32 version --- src/global.c | 129 ++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 92 insertions(+), 37 deletions(-) diff --git a/src/global.c b/src/global.c index 4f024f17e..fe3a3c8b8 100644 --- a/src/global.c +++ b/src/global.c @@ -18,6 +18,7 @@ git_mutex git__mwindow_mutex; git_global_shutdown_fn git__shutdown_callbacks[MAX_SHUTDOWN_CB]; git_atomic git__n_shutdown_callbacks; +git_atomic git__n_inits; void git__on_shutdown(git_global_shutdown_fn callback) { @@ -73,47 +74,99 @@ static void git__shutdown(void) #if defined(GIT_THREADS) && defined(GIT_WIN32) static DWORD _tls_index; -static int _tls_init = 0; +static HANDLE _init_mutex = NULL; +static HANDLE _initialized_event = NULL; -int git_threads_init(void) +static int synchronized_init_and_claim_mutex(volatile HANDLE *m) +{ + if (*m == NULL) { + HANDLE nm = CreateMutex(NULL, FALSE, NULL); + if (InterlockedCompareExchangePointer(m, nm, NULL) != NULL) { + CloseHandle(nm); + } + } + return WaitForSingleObject(*m, INFINITE) == WAIT_FAILED; +} + +static int synchronized_threads_init() { int error; - if (_tls_init) - return 0; + if (synchronized_init_and_claim_mutex(&_init_mutex) != WAIT_OBJECT_0) { + giterr_set(GITERR_OS, "git_threads_init: WaitForSingleObject failed"); + return -1; + } + + if (!_initialized_event) { + _initialized_event = CreateEvent(NULL, TRUE, FALSE, NULL); + } _tls_index = TlsAlloc(); if (git_mutex_init(&git__mwindow_mutex)) return -1; /* Initialize any other subsystems that have global state */ - if ((error = git_hash_global_init()) >= 0 && - (error = git_futils_dirs_global_init()) >= 0) - _tls_init = 1; - - GIT_MEMORY_BARRIER; + if ((error = git_hash_global_init()) >= 0) + error = git_futils_dirs_global_init(); win32_pthread_initialize(); - + SetEvent(_initialized_event); + ReleaseMutex(_init_mutex); return error; } +int git_threads_init(void) +{ + int error = -1, + n_inits = git_atomic_inc(&git__n_inits); + + if (n_inits == 1) + error = synchronized_threads_init(); + else { + while (!_initialized_event); /* SPIN */ + error = (WaitForSingleObject(_initialized_event, INFINITE) == WAIT_OBJECT_0) ? 0 : -1; + } + + GIT_MEMORY_BARRIER; + return error; +} + +static void synchronized_threads_shutdown() +{ + if (WaitForSingleObject(_init_mutex, INFINITE) != WAIT_OBJECT_0) { + giterr_set(GITERR_OS, "git_threads_shutdown: WaitForSingleObject failed"); + return; + } + + if (git__n_inits.val != 0) { + /* Someone else initialized while we were waiting */ + ReleaseMutex(_init_mutex); + return; + } + ResetEvent(_initialized_event); + + /* Shut down any subsystems that have global state */ + git__shutdown(); + TlsFree(_tls_index); + git_mutex_free(&git__mwindow_mutex); + + ReleaseMutex(_init_mutex); +} + void git_threads_shutdown(void) { - /* Shut down any subsystems that have global state */ - git__shutdown(); + int n_inits = git_atomic_dec(&git__n_inits); - TlsFree(_tls_index); - _tls_init = 0; - - git_mutex_free(&git__mwindow_mutex); + if (n_inits == 0) + synchronized_threads_shutdown(); + GIT_MEMORY_BARRIER; } git_global_st *git__global_state(void) { void *ptr; - assert(_tls_init); + assert(git__n_inits.val); if ((ptr = TlsGetValue(_tls_index)) != NULL) return ptr; @@ -130,55 +183,57 @@ git_global_st *git__global_state(void) #elif defined(GIT_THREADS) && defined(_POSIX_THREADS) static pthread_key_t _tls_key; -static int _tls_init = 0; +static pthread_once_t _once_init = PTHREAD_ONCE_INIT; +int init_error = 0; static void cb__free_status(void *st) { git__free(st); } -int git_threads_init(void) +static void init_once(void) { - int error = 0; - - if (_tls_init) - return 0; - - if (git_mutex_init(&git__mwindow_mutex)) - return -1; + if ((init_error = git_mutex_init(&git__mwindow_mutex)) != 0) + return; pthread_key_create(&_tls_key, &cb__free_status); /* Initialize any other subsystems that have global state */ - if ((error = git_hash_global_init()) >= 0 && - (error = git_futils_dirs_global_init()) >= 0) - _tls_init = 1; + if ((init_error = git_hash_global_init()) >= 0) + init_error = git_futils_dirs_global_init(); GIT_MEMORY_BARRIER; +} - return error; +int git_threads_init(void) +{ + pthread_once(&_once_init, init_once); + git_atomic_inc(&git__n_inits); + return init_error; } void git_threads_shutdown(void) { + pthread_once_t new_once = PTHREAD_ONCE_INIT; + + if (git_atomic_dec(&git__n_inits) > 0) return; + /* Shut down any subsystems that have global state */ git__shutdown(); - if (_tls_init) { - void *ptr = pthread_getspecific(_tls_key); - pthread_setspecific(_tls_key, NULL); - git__free(ptr); - } + void *ptr = pthread_getspecific(_tls_key); + pthread_setspecific(_tls_key, NULL); + git__free(ptr); pthread_key_delete(_tls_key); - _tls_init = 0; git_mutex_free(&git__mwindow_mutex); + _once_init = new_once; } git_global_st *git__global_state(void) { void *ptr; - assert(_tls_init); + assert(git__n_inits.val); if ((ptr = pthread_getspecific(_tls_key)) != NULL) return ptr; From cdc95a0d93aa3b515f9c4fe30f47d52e8e7f3dde Mon Sep 17 00:00:00 2001 From: Philip Kelley Date: Fri, 4 Oct 2013 18:38:37 -0400 Subject: [PATCH 3/4] Use InterlockedCompareExchange for the lock --- src/global.c | 67 ++++++++++++++-------------------------------------- 1 file changed, 18 insertions(+), 49 deletions(-) diff --git a/src/global.c b/src/global.c index fe3a3c8b8..33a760c54 100644 --- a/src/global.c +++ b/src/global.c @@ -74,33 +74,12 @@ static void git__shutdown(void) #if defined(GIT_THREADS) && defined(GIT_WIN32) static DWORD _tls_index; -static HANDLE _init_mutex = NULL; -static HANDLE _initialized_event = NULL; - -static int synchronized_init_and_claim_mutex(volatile HANDLE *m) -{ - if (*m == NULL) { - HANDLE nm = CreateMutex(NULL, FALSE, NULL); - if (InterlockedCompareExchangePointer(m, nm, NULL) != NULL) { - CloseHandle(nm); - } - } - return WaitForSingleObject(*m, INFINITE) == WAIT_FAILED; -} +static DWORD _mutex = 0; static int synchronized_threads_init() { int error; - if (synchronized_init_and_claim_mutex(&_init_mutex) != WAIT_OBJECT_0) { - giterr_set(GITERR_OS, "git_threads_init: WaitForSingleObject failed"); - return -1; - } - - if (!_initialized_event) { - _initialized_event = CreateEvent(NULL, TRUE, FALSE, NULL); - } - _tls_index = TlsAlloc(); if (git_mutex_init(&git__mwindow_mutex)) return -1; @@ -110,56 +89,46 @@ static int synchronized_threads_init() error = git_futils_dirs_global_init(); win32_pthread_initialize(); - SetEvent(_initialized_event); - ReleaseMutex(_init_mutex); + return error; } int git_threads_init(void) { - int error = -1, - n_inits = git_atomic_inc(&git__n_inits); + int error = 0; - if (n_inits == 1) + /* Enter the lock */ + while (InterlockedCompareExchange(&_mutex, 1, 0)) { Sleep(0); } + + /* Only do work on a 0 -> 1 transition of the refcount */ + if (1 == ++git__n_inits.val) error = synchronized_threads_init(); - else { - while (!_initialized_event); /* SPIN */ - error = (WaitForSingleObject(_initialized_event, INFINITE) == WAIT_OBJECT_0) ? 0 : -1; - } - GIT_MEMORY_BARRIER; + /* Exit the lock */ + InterlockedExchange(&_mutex, 0); + return error; } static void synchronized_threads_shutdown() { - if (WaitForSingleObject(_init_mutex, INFINITE) != WAIT_OBJECT_0) { - giterr_set(GITERR_OS, "git_threads_shutdown: WaitForSingleObject failed"); - return; - } - - if (git__n_inits.val != 0) { - /* Someone else initialized while we were waiting */ - ReleaseMutex(_init_mutex); - return; - } - ResetEvent(_initialized_event); - /* Shut down any subsystems that have global state */ git__shutdown(); TlsFree(_tls_index); git_mutex_free(&git__mwindow_mutex); - - ReleaseMutex(_init_mutex); } void git_threads_shutdown(void) { - int n_inits = git_atomic_dec(&git__n_inits); + /* Enter the lock */ + while (InterlockedCompareExchange(&_mutex, 1, 0)) { Sleep(0); } - if (n_inits == 0) + /* Only do work on a 1 -> 0 transition of the refcount */ + if (0 == --git__n_inits.val) synchronized_threads_shutdown(); - GIT_MEMORY_BARRIER; + + /* Exit the lock */ + InterlockedExchange(&_mutex, 0); } git_global_st *git__global_state(void) From 22661448973e3c46f2a71a75364630a5111f7c14 Mon Sep 17 00:00:00 2001 From: Ben Straub Date: Fri, 4 Oct 2013 19:35:32 -0700 Subject: [PATCH 4/4] Don't use git_atomic as an integer --- src/global.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/global.c b/src/global.c index 33a760c54..7d39c6fa8 100644 --- a/src/global.c +++ b/src/global.c @@ -18,7 +18,6 @@ git_mutex git__mwindow_mutex; git_global_shutdown_fn git__shutdown_callbacks[MAX_SHUTDOWN_CB]; git_atomic git__n_shutdown_callbacks; -git_atomic git__n_inits; void git__on_shutdown(git_global_shutdown_fn callback) { @@ -75,6 +74,7 @@ static void git__shutdown(void) static DWORD _tls_index; static DWORD _mutex = 0; +static DWORD _n_inits = 0; static int synchronized_threads_init() { @@ -101,7 +101,7 @@ int git_threads_init(void) while (InterlockedCompareExchange(&_mutex, 1, 0)) { Sleep(0); } /* Only do work on a 0 -> 1 transition of the refcount */ - if (1 == ++git__n_inits.val) + if (1 == ++_n_inits) error = synchronized_threads_init(); /* Exit the lock */ @@ -124,7 +124,7 @@ void git_threads_shutdown(void) while (InterlockedCompareExchange(&_mutex, 1, 0)) { Sleep(0); } /* Only do work on a 1 -> 0 transition of the refcount */ - if (0 == --git__n_inits.val) + if (0 == --_n_inits) synchronized_threads_shutdown(); /* Exit the lock */ @@ -135,7 +135,7 @@ git_global_st *git__global_state(void) { void *ptr; - assert(git__n_inits.val); + assert(_n_inits); if ((ptr = TlsGetValue(_tls_index)) != NULL) return ptr; @@ -153,6 +153,7 @@ git_global_st *git__global_state(void) static pthread_key_t _tls_key; static pthread_once_t _once_init = PTHREAD_ONCE_INIT; +static git_atomic git__n_inits; int init_error = 0; static void cb__free_status(void *st)