You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@wandisco.com> on 2011/03/15 14:52:22 UTC
Re: Race condition in libsvn_client code?
"C. Michael Pilato" <cm...@collab.net> writes:
> [Tweaking Subject: for (hopefully) additional visibility.]
>
> On 03/15/2011 09:15 AM, Daniel Shahaf wrote:
>> C. Michael Pilato wrote on Tue, Mar 15, 2011 at 09:05:56 -0400:
>>> On 03/15/2011 12:34 AM, Daniel Shahaf wrote:
>>>> cmpilato@apache.org wrote on Tue, Mar 08, 2011 at 20:05:51 -0000:
>>>>> Author: cmpilato
>>>>> Date: Tue Mar 8 20:05:50 2011
>>>>> New Revision: 1079508
>>>
>>> [...]
>>>
>>>>> svn_client_commit4(svn_commit_info_t **commit_info_p,
>>>>> const apr_array_header_t *targets,
>>>
>>> [...]
>>>
>>>>> + /* Ensure that the original notification system is in place. */
>>>>> + ctx->notify_func2 = notify_baton.orig_notify_func2;
>>>>> + ctx->notify_baton2 = notify_baton.orig_notify_baton2;
>>>>> +
>>>>
>>>> This is actually a race condition, isn't it? (for clients that call the
>>>> deprecated API while using CTX->notify_func2 in another thread (in the
>>>> same or another API))
>>>>
>>>> (Okay, so maybe we'll just let it live on. Presumably N other
>>>> deprecated wrappers do this too.)
>>>
>>> I hadn't considered that. We do alot of pointer swaps like this up in the
>>> command-line client code itself, but that's a bit different than doing so
>>> down in the libsvn_client library as in this case. There is one prior
>>> instance of us doing this kind of swap inside the libsvn_client library: in
>>> libsvn_client/copy.c:repos_to_wc_copy_single(). But I'd prefer mere
>>> precedent not to be our reason for allowing badness to persist in the codebase.
>>>
>>> Got suggestions?
>>
>> Would this work? ---
>>
>> svn_client_ctx_t ctx2 = *ctx;
>> ctx2.notify_func2 = notify_baton.orig_notify_func2;
>> ctx2.notify_baton2 = notify_baton.orig_notify_baton2;
>> svn_client_commit5(ctx=&ctx2);
It has a different problem, if the client modifies the context in a
callback those modifications will not persist.
--
Philip
RE: Race condition in libsvn_client code?
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Philip Martin [mailto:philip.martin@wandisco.com]
> Sent: dinsdag 15 maart 2011 14:52
> To: C. Michael Pilato
> Cc: Subversion Development
> Subject: Re: Race condition in libsvn_client code?
>
> "C. Michael Pilato" <cm...@collab.net> writes:
>
> > [Tweaking Subject: for (hopefully) additional visibility.]
> >
> > On 03/15/2011 09:15 AM, Daniel Shahaf wrote:
> >> C. Michael Pilato wrote on Tue, Mar 15, 2011 at 09:05:56 -0400:
> >>> On 03/15/2011 12:34 AM, Daniel Shahaf wrote:
> >>>> cmpilato@apache.org wrote on Tue, Mar 08, 2011 at 20:05:51 -0000:
> >>>>> Author: cmpilato
> >>>>> Date: Tue Mar 8 20:05:50 2011
> >>>>> New Revision: 1079508
> >>>
> >>> [...]
> >>>
> >>>>> svn_client_commit4(svn_commit_info_t **commit_info_p,
> >>>>> const apr_array_header_t *targets,
> >>>
> >>> [...]
> >>>
> >>>>> + /* Ensure that the original notification system is in place. */
> >>>>> + ctx->notify_func2 = notify_baton.orig_notify_func2;
> >>>>> + ctx->notify_baton2 = notify_baton.orig_notify_baton2;
> >>>>> +
> >>>>
> >>>> This is actually a race condition, isn't it? (for clients that call
the
> >>>> deprecated API while using CTX->notify_func2 in another thread (in
> the
> >>>> same or another API))
> >>>>
> >>>> (Okay, so maybe we'll just let it live on. Presumably N other
> >>>> deprecated wrappers do this too.)
> >>>
> >>> I hadn't considered that. We do alot of pointer swaps like this up in
the
> >>> command-line client code itself, but that's a bit different than doing
so
> >>> down in the libsvn_client library as in this case. There is one prior
> >>> instance of us doing this kind of swap inside the libsvn_client
library: in
> >>> libsvn_client/copy.c:repos_to_wc_copy_single(). But I'd prefer mere
> >>> precedent not to be our reason for allowing badness to persist in the
> codebase.
> >>>
> >>> Got suggestions?
> >>
> >> Would this work? ---
> >>
> >> svn_client_ctx_t ctx2 = *ctx;
> >> ctx2.notify_func2 = notify_baton.orig_notify_func2;
> >> ctx2.notify_baton2 = notify_baton.orig_notify_baton2;
> >> svn_client_commit5(ctx=&ctx2);
>
> It has a different problem, if the client modifies the context in a
> callback those modifications will not persist.
I don't think any of our callbacks receives the current ctx instance.
But see my other mail: We store a svn_wc_context_t in the client context. So
svn_client_ctx_t can't be shared between functions running on different
threads anyway.
Bert