> -----Original Message-----
> From: Philip Martin [mailto:philip.martin_at_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" <cmpilato_at_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_at_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
Received on 2011-03-15 14:59:23 CET