On Mon, May 5, 2014 at 2:57 PM, Ivan Zhakov <i...@visualsvn.com> wrote:
> On 5 May 2014 16:47, Bert Huijben <b...@qqmail.nl> wrote: > > > > > >> -----Original Message----- > >> From: stef...@apache.org [mailto:stef...@apache.org] > >> Sent: vrijdag 2 mei 2014 16:08 > >> To: comm...@subversion.apache.org > >> Subject: svn commit: r1591919 - in /subversion/trunk/subversion: > include/ > >> include/private/ libsvn_fs/ libsvn_fs_base/bdb/ libsvn_fs_fs/ > libsvn_fs_x/ > >> libsvn_ra_svn/ libsvn_subr/ svnserve/ tests/ tests/libsvn_fs_fs/ > >> > >> Author: stefan2 > >> Date: Fri May 2 14:07:40 2014 > >> New Revision: 1591919 > >> > >> URL: http://svn.apache.org/r1591919 > >> Log: > >> APR mutexes don't support recursive locking on all platforms. > >> As a result, trying to take out the same lock twice in the > >> same thread will cause a lock up under e.g. Linux. This patch > >> adds an option to svn_mutex__t that detects recursive locking > >> attempts in most cases and returns a proper error. > >> > >> The idea is simply to store the thread ID of the lock OWNER > >> along the actual mutex object. If that matches the current > >> thread's ID, there is a violation. As the current thread > >> cannot race with itself and because any other thread uses a > >> different thread ID, setting and comparing this aux. info can > >> be done safely. > >> > >> Also, we may allow for false negatives here since we only try > >> to detect code sequences that are already illegal in the first > >> place. We also don't make that check mandatory as access to > >> thread IDs and their comparison may be somewhat expensive on > >> some systems - which would impair the futex-like behavior that > >> we assume in some places like the caches. > >> > >> A more detailed description has been added to the source code. > >> A FSFS-based test is added as that has been the origin of the > >> feature request. > >> > >> Current users will be updated as follows. FS level locks and > >> library / module initialization will enable recursion detection. > >> Potentially runtime critical, internal use disables it. > >> > >> * subversion/include/svn_error_codes.h > >> (SVN_ERR_RECURSIVE_LOCK): Define a new error code for invalid > >> locking schemes. > >> > >> * subversion/include/private/svn_mutex.h > >> (svn_mutex__t): Our mutex is now a struct as we add aux. > >> data to it. > >> (svn_mutex__init): Add the CHECKED option. > >> > >> * subversion/libsvn_subr/mutex.c > >> (svn_mutex__t): Define the mutex structure and and document > >> the aux. data extensively. > >> (svn_mutex__init): Update constructor. > >> (svn_mutex__lock): Optionally, check for recursive locking attempts > >> and update the associtated aux. data. > >> (svn_mutex__unlock): Optionally, update the aux. data for recursive > >> lock detection. > >> > >> * subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c > >> (never_reached, > >> lock_again): Callback functions required by the new test. > >> (recursive_locking): New test expecting an SVN_ERR_RECURSIVE_LOCK. > >> (test_funcs): Register the new test. > > > > This new test currently causes a deadlock on Windows and a test failure > on OpenBSD. > > > [...] > > > Windows: > > Hang in > > [[[ > >> libapr-1.dll!apr_file_lock(apr_file_t * thefile, int type) Line > 35 C > > fs-fs-pack-test.exe!svn_io_lock_open_file(apr_file_t * > lockfile_handle, int exclusive, int nonblocking, apr_pool_t * pool) Line > 2137 C > > fs-fs-pack-test.exe!svn_io_file_lock2(const char * lock_file, > int exclusive, int nonblocking, apr_pool_t * pool) Line 2239 C > > fs-fs-pack-test.exe!get_lock_on_filesystem(const char * > lock_filename, apr_pool_t * pool) Line 115 C > > fs-fs-pack-test.exe!with_some_lock_file(with_lock_baton_t * > baton) Line 202 C > > fs-fs-pack-test.exe!with_lock(void * baton, apr_pool_t * pool) > Line 248 C > > fs-fs-pack-test.exe!svn_fs_fs__with_all_locks(svn_fs_t * fs, > svn_error_t * (void *, apr_pool_t *) * body, void * baton, apr_pool_t * > pool) Line 424 C > > fs-fs-pack-test.exe!lock_again(void * baton, apr_pool_t * pool) > Line 1148 C > > fs-fs-pack-test.exe!with_some_lock_file(with_lock_baton_t * > baton) Line 230 C > > fs-fs-pack-test.exe!with_lock(void * baton, apr_pool_t * pool) > Line 248 C > > fs-fs-pack-test.exe!with_some_lock_file(with_lock_baton_t * > baton) Line 230 C > > fs-fs-pack-test.exe!with_lock(void * baton, apr_pool_t * pool) > Line 248 C > > fs-fs-pack-test.exe!with_some_lock_file(with_lock_baton_t * > baton) Line 230 C > > fs-fs-pack-test.exe!with_lock(void * baton, apr_pool_t * pool) > Line 248 C > > fs-fs-pack-test.exe!svn_fs_fs__with_all_locks(svn_fs_t * fs, > svn_error_t * (void *, apr_pool_t *) * body, void * baton, apr_pool_t * > pool) Line 424 C > > fs-fs-pack-test.exe!recursive_locking(const svn_test_opts_t * > opts, apr_pool_t * pool) Line 1159 C > > fs-fs-pack-test.exe!test_thread(apr_thread_t * thread, void * > data) Line 519 C > > libapr-1.dll!dummy_worker(void * opaque) Line 79 C > > ]]] > > > Most likely this happens because file locks are *per file handle* on > Windows: not per-process as on posix. That means that one thread > cannot obtain lock to file using different file handles. > Well, it should not get to the point where it would attempt to take out the second file lock. The recursive mutex lock should be detected prior to that. Could you have a lock what thread IDs we get in svn_mutex__lock? This is strictly single-threaded code and should be easy to debug. I simply have no windows machine available ATM. -- Stefan^2.
