On Mon, May 5, 2014 at 2:57 PM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
> On 5 May 2014 16:47, Bert Huijben <bert_at_qqmail.nl> wrote:
> >
> >
> >> -----Original Message-----
> >> From: stefan2_at_apache.org [mailto:stefan2_at_apache.org]
> >> Sent: vrijdag 2 mei 2014 16:08
> >> To: commits_at_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.
Received on 2014-05-06 00:22:32 CEST