You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2010/01/22 23:50:43 UTC
svn commit: r902303 - in /subversion/trunk/subversion:
include/private/svn_wc_private.h libsvn_wc/lock.c
Author: stsp
Date: Fri Jan 22 22:50:42 2010
New Revision: 902303
URL: http://svn.apache.org/viewvc?rev=902303&view=rev
Log:
Add a safe API for using write locks on a working copy,
i.e. locks acquired via svn_wc__acquire_write_lock() and
released via svn_wc__release_write_lock().
Quoting Philip Martin from
http://mail-archives.apache.org/mod_mbox/subversion-dev/200912.mbox/%3C87ws0g458j.fsf@stat.home.lan%3E
because he explained the problem very well:
In 1.6 wc locks were associated with access batons and when an access
baton was opened a pool cleanup handler was installed to ensure that
the baton was closed. The pool cleanup handler would remove any lock
associated with the access baton provided there were no log files in
the directory. Code was typically:
svn_wc_adm_access_t *adm_access;
SVN_ERR(svn_wc_adm_open3(&adm_access, ... pool));
SVN_ERR(svn_wc_foo(adm_access, ... pool));
SVN_ERR(svn_wc_adm_close(adm_access));
If svn_wc_foo completes without error then svn_wc_adm_close is called
which removes the locks unconditionally. If svn_wc_foo returns an
error then svn_wc_adm_close doesn't get called, and when the pool is
finally cleared the pool cleanup handler will leave or remove the lock
depending on whether cleanup is required.
End of qoute.
Quoting Greg Stein from #svn-dev:
<gstein> unlocking via pool cleanup blows
<gstein> "is the database still open? can I manipulate stuff in it?"
<gstein> is only the first and worst of the questions when you try
to do shit like that in the cleanup
End of qoute.
Instead of releasing wc-ng write locks during pool cleanup,
we provide a function which invokes a callback while holding a
wc lock and releases the lock after the callback returns.
We do not anticipate many users of this type of lock, so use of
this API should be limited to few occurrences within libsvn_client.
This API is private for now, as we may still decide to change it.
Suggested by: gstein
hwright
* subversion/include/private/svn_wc_private.h
(svn_wc__with_write_lock_func_t): New callback type.
(svn_wc__call_with_write_lock): Declare.
* subversion/libsvn_wc/lock.c
(svn_wc__call_with_write_lock): New.
Modified:
subversion/trunk/subversion/include/private/svn_wc_private.h
subversion/trunk/subversion/libsvn_wc/lock.c
Modified: subversion/trunk/subversion/include/private/svn_wc_private.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private/svn_wc_private.h?rev=902303&r1=902302&r2=902303&view=diff
==============================================================================
--- subversion/trunk/subversion/include/private/svn_wc_private.h (original)
+++ subversion/trunk/subversion/include/private/svn_wc_private.h Fri Jan 22 22:50:42 2010
@@ -542,6 +542,26 @@
const char *local_abspath,
apr_pool_t *scratch_pool);
+/** A callback invoked by the svn_wc__call_with_write_lock() function. */
+typedef svn_error_t *(*svn_wc__with_write_lock_func_t)(void *baton,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool);
+
+
+/** Call function @a func while holding a write lock on
+ * @a local_abspath. The @a baton, and @a result_pool and
+ * @a scratch_pool, is passed @a func.
+ * Use @a wc_ctx for working copy access.
+ * The lock is guaranteed to be released after @a func returns.
+ */
+svn_error_t *
+svn_wc__call_with_write_lock(svn_wc__with_write_lock_func_t func,
+ void *baton,
+ svn_wc_context_t *wc_ctx,
+ const char *local_abspath,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool);
+
#ifdef __cplusplus
}
#endif /* __cplusplus */
Modified: subversion/trunk/subversion/libsvn_wc/lock.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/lock.c?rev=902303&r1=902302&r2=902303&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/lock.c (original)
+++ subversion/trunk/subversion/libsvn_wc/lock.c Fri Jan 22 22:50:42 2010
@@ -1848,3 +1848,18 @@
return SVN_NO_ERROR;
}
+
+svn_error_t *
+svn_wc__call_with_write_lock(svn_wc__with_write_lock_func_t func,
+ void *baton,
+ svn_wc_context_t *wc_ctx,
+ const char *local_abspath,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
+{
+ SVN_ERR(svn_wc__acquire_write_lock(NULL, wc_ctx, local_abspath,
+ scratch_pool, scratch_pool));
+ return svn_error_compose_create(
+ func(baton, result_pool, scratch_pool),
+ svn_wc__release_write_lock(wc_ctx, local_abspath, scratch_pool));
+}
Re: svn commit: r902303 - in /subversion/trunk/subversion:
include/private/svn_wc_private.h libsvn_wc/lock.c
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Jan 27, 2010 at 10:35:55AM +0000, Philip Martin wrote:
> Stefan Sperling <st...@elego.de> writes:
> > Can we apply your patch to fix the ordering problem?
>
> I think we should apply it, but the patch tests fail.
I will take a look.
> It raises the question: why are the patch tests taking a lock? They
> seem to pass without the lock. Does this mean the lock is
> unnecessary? Or does it mean that there is some code that requires a
> lock but is not checking to make sure that a lock is held?
The patch can modify the working copy in arbitrary ways.
It can delete files or directories, add new ones, etc.
The lock is held to say "I am starting a long-running operation
modifying the working copy and I don't know what the end result
will be yet! Everyone else please back off."
Stefan
Re: svn commit: r902303 - in /subversion/trunk/subversion: include/private/svn_wc_private.h libsvn_wc/lock.c
Posted by Philip Martin <ph...@wandisco.com>.
Stefan Sperling <st...@elego.de> writes:
>> --- subversion/libsvn_wc/lock.c (revision 903527)
>> +++ subversion/libsvn_wc/lock.c (working copy)
>> @@ -1857,9 +1857,10 @@
>> apr_pool_t *result_pool,
>> apr_pool_t *scratch_pool)
>> {
>> + svn_error_t *err1, *err2;
>> SVN_ERR(svn_wc__acquire_write_lock(NULL, wc_ctx, local_abspath,
>> scratch_pool, scratch_pool));
>> - return svn_error_compose_create(
>> - func(baton, result_pool, scratch_pool),
>> - svn_wc__release_write_lock(wc_ctx, local_abspath, scratch_pool));
>> + err1 = func(baton, result_pool, scratch_pool);
>> + err2 = svn_wc__release_write_lock(wc_ctx, local_abspath, scratch_pool);
>> + return svn_error_compose_create(err1, err2);
>> }
>>
>> However, if I make that change I get several test failures:
>
> Are these related to the svn_wc_notify_t-related issues you found?
No, those were separate and are now fixed.
> Can we apply your patch to fix the ordering problem?
I think we should apply it, but the patch tests fail.
It raises the question: why are the patch tests taking a lock? They
seem to pass without the lock. Does this mean the lock is
unnecessary? Or does it mean that there is some code that requires a
lock but is not checking to make sure that a lock is held?
--
Philip
Re: svn commit: r902303 - in /subversion/trunk/subversion:
include/private/svn_wc_private.h libsvn_wc/lock.c
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Jan 27, 2010 at 02:39:05AM +0000, Philip Martin wrote:
> stsp@apache.org writes:
>
> > Author: stsp
> > Date: Fri Jan 22 22:50:42 2010
> > New Revision: 902303
>
> > +svn_error_t *
> > +svn_wc__call_with_write_lock(svn_wc__with_write_lock_func_t func,
> > + void *baton,
> > + svn_wc_context_t *wc_ctx,
> > + const char *local_abspath,
> > + apr_pool_t *result_pool,
> > + apr_pool_t *scratch_pool)
> > +{
> > + SVN_ERR(svn_wc__acquire_write_lock(NULL, wc_ctx, local_abspath,
> > + scratch_pool, scratch_pool));
> > + return svn_error_compose_create(
> > + func(baton, result_pool, scratch_pool),
> > + svn_wc__release_write_lock(wc_ctx, local_abspath, scratch_pool));
> > +}
>
> That's not going to work because the two function calls func() and
> svn_wc__release_write_lock() are not evaluated in any particular
> order.
Oh, the order depends on the compiler, right?
Good catch.
> It needs to be something like:
>
> Index: subversion/libsvn_wc/lock.c
> ===================================================================
> --- subversion/libsvn_wc/lock.c (revision 903527)
> +++ subversion/libsvn_wc/lock.c (working copy)
> @@ -1857,9 +1857,10 @@
> apr_pool_t *result_pool,
> apr_pool_t *scratch_pool)
> {
> + svn_error_t *err1, *err2;
> SVN_ERR(svn_wc__acquire_write_lock(NULL, wc_ctx, local_abspath,
> scratch_pool, scratch_pool));
> - return svn_error_compose_create(
> - func(baton, result_pool, scratch_pool),
> - svn_wc__release_write_lock(wc_ctx, local_abspath, scratch_pool));
> + err1 = func(baton, result_pool, scratch_pool);
> + err2 = svn_wc__release_write_lock(wc_ctx, local_abspath, scratch_pool);
> + return svn_error_compose_create(err1, err2);
> }
>
> However, if I make that change I get several test failures:
Are these related to the svn_wc_notify_t-related issues you found?
Can we apply your patch to fix the ordering problem?
Stefan
Re: svn commit: r902303 - in /subversion/trunk/subversion: include/private/svn_wc_private.h libsvn_wc/lock.c
Posted by Philip Martin <ph...@wandisco.com>.
stsp@apache.org writes:
> Author: stsp
> Date: Fri Jan 22 22:50:42 2010
> New Revision: 902303
> +svn_error_t *
> +svn_wc__call_with_write_lock(svn_wc__with_write_lock_func_t func,
> + void *baton,
> + svn_wc_context_t *wc_ctx,
> + const char *local_abspath,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> +{
> + SVN_ERR(svn_wc__acquire_write_lock(NULL, wc_ctx, local_abspath,
> + scratch_pool, scratch_pool));
> + return svn_error_compose_create(
> + func(baton, result_pool, scratch_pool),
> + svn_wc__release_write_lock(wc_ctx, local_abspath, scratch_pool));
> +}
That's not going to work because the two function calls func() and
svn_wc__release_write_lock() are not evaluated in any particular
order. It needs to be something like:
Index: subversion/libsvn_wc/lock.c
===================================================================
--- subversion/libsvn_wc/lock.c (revision 903527)
+++ subversion/libsvn_wc/lock.c (working copy)
@@ -1857,9 +1857,10 @@
apr_pool_t *result_pool,
apr_pool_t *scratch_pool)
{
+ svn_error_t *err1, *err2;
SVN_ERR(svn_wc__acquire_write_lock(NULL, wc_ctx, local_abspath,
scratch_pool, scratch_pool));
- return svn_error_compose_create(
- func(baton, result_pool, scratch_pool),
- svn_wc__release_write_lock(wc_ctx, local_abspath, scratch_pool));
+ err1 = func(baton, result_pool, scratch_pool);
+ err2 = svn_wc__release_write_lock(wc_ctx, local_abspath, scratch_pool);
+ return svn_error_compose_create(err1, err2);
}
However, if I make that change I get several test failures:
$ ../../../../src/subversion/tests/cmdline/patch_tests.py --parallel
F.FF.F.
The tests pass if I reverse the two function calls but obviously that
makes the locking pointless:
Index: subversion/libsvn_wc/lock.c
===================================================================
--- subversion/libsvn_wc/lock.c (revision 903527)
+++ subversion/libsvn_wc/lock.c (working copy)
@@ -1857,9 +1857,10 @@
apr_pool_t *result_pool,
apr_pool_t *scratch_pool)
{
+ svn_error_t *err1, *err2;
SVN_ERR(svn_wc__acquire_write_lock(NULL, wc_ctx, local_abspath,
scratch_pool, scratch_pool));
- return svn_error_compose_create(
- func(baton, result_pool, scratch_pool),
- svn_wc__release_write_lock(wc_ctx, local_abspath, scratch_pool));
+ err2 = svn_wc__release_write_lock(wc_ctx, local_abspath, scratch_pool);
+ err1 = func(baton, result_pool, scratch_pool);
+ return svn_error_compose_create(err1, err2);
}
--
Philip