You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@apache.org> on 2018/07/02 11:20:11 UTC
Re: svn commit: r1834612 - in
/subversion/branches/1.10.x-issue4758/subversion: libsvn_client/shelve.c
tests/cmdline/shelve_tests.py
There are three issues here:
1.
Philip Martin wrote:
> There are SVN_ERR between these two hunks. If one of these returns an
> error we will fail to restore the old config.
There are four other places where libsvn_client functions modify ctx in place. They are all modifying the notification callback, with the following pattern:
modify ctx->xxx
err = func();
restore ctx->xxx
SVN_ERR(err);
To solve that problem I will use the same pattern as above.
r1834835, added to backport nomination in r1834836.
Note that this fix is for the 1.10.x branch only, as this bit of shelving code doesn't exist on trunk.
2.
> I did also worry about thread safety: it's not safe to modify the
> context like that if the context can be shared across multiple threads.
> However the context also includes batons and those typically point to
> mutable data which cannot be shared across threads either. I suppose we
> should document that a client should only access a context from one
> thread at a time.
Documenting that sounds reasonable. Like this, just above typedef struct svn_client_ctx_t, do you think?
[[[
/**
* Client context
*
+ * A client should only access a context from one thread at a time, as
+ * the context includes batons that typically point to mutable data which
+ * cannot be safely shared across threads.
+ *
* @defgroup clnt_ctx Client context management
*
* @{
*/
]]]
3.
>> Bert Huijben writes:
>>> This resets quite a bit more configuration that might still be
>>> necessary. [...]
>>> I think you should duplicate the hash and explicitly remove a few keys.
>
> If we do remove keys then I think DIFF_CMD and DIFF_EXTENSIONS are the
> ones.
That seems like the correct form of solution, in general.
In this case, however, the only reference to 'ctx->config' in the whole of libsvn_client/diff.c is passing it to create_diff_writer_info() to configure an external diff tool, and I don't think it is used by any called functions either.
So I think this is OK as it is.
- Julian
Re: svn commit: r1834612 - in
/subversion/branches/1.10.x-issue4758/subversion: libsvn_client/shelve.c
tests/cmdline/shelve_tests.py
Posted by Branko Čibej <br...@apache.org>.
On 13.07.2018 17:36, Philip Martin wrote:
> Julian Foad <ju...@apache.org> writes:
>
>> Philip, can you help clarify this?
>>> It's not clear to me from the added paragraph whether the restriction
>>> "only access the context from a thread at a time" is imposed by the
>>> library implementation or by the way API consumers' code is typically
>>> structured. That is: if a client is careful to use mutexes in his
>>> batons, or for that matter passes NULL for all batons, would the
>>> restriction still apply?
> Look at repos_to_wc_copy_single() for example: it stores the original
> notify func/baton, sets temporary values, then restores the original
> values. I think it is clear that only works if the context is for
> single thread, even if the func/batons are NULL.
We've always assumed that our threading model expects reentrant
functions but restricted shared structures, such as client context, WC
context, RA context etc. But we've not done a very good job at actually
documenting that.
-- Brane
Re: svn commit: r1834612 - in /subversion/branches/1.10.x-issue4758/subversion: libsvn_client/shelve.c tests/cmdline/shelve_tests.py
Posted by Philip Martin <ph...@codematters.co.uk>.
Julian Foad <ju...@apache.org> writes:
> Philip, can you help clarify this?
>>
>> It's not clear to me from the added paragraph whether the restriction
>> "only access the context from a thread at a time" is imposed by the
>> library implementation or by the way API consumers' code is typically
>> structured. That is: if a client is careful to use mutexes in his
>> batons, or for that matter passes NULL for all batons, would the
>> restriction still apply?
Look at repos_to_wc_copy_single() for example: it stores the original
notify func/baton, sets temporary values, then restores the original
values. I think it is clear that only works if the context is for
single thread, even if the func/batons are NULL.
--
Philip
Re: svn commit: r1834612 - in
/subversion/branches/1.10.x-issue4758/subversion: libsvn_client/shelve.c
tests/cmdline/shelve_tests.py
Posted by Julian Foad <ju...@apache.org>.
Philip, can you help clarify this?
Daniel Shahaf wrote on 2018-07-02:
> Julian Foad wrote on Mon, 02 Jul 2018 12:20 +0100:
> > Philip Martin wrote:
> > > I did also worry about thread safety: it's not safe to modify the
> > > context like that if the context can be shared across multiple threads.
> > > However the context also includes batons and those typically point to
> > > mutable data which cannot be shared across threads either. I suppose we
> > > should document that a client should only access a context from one
> > > thread at a time.
> >
> > Documenting that sounds reasonable. Like this, just above typedef struct
> > svn_client_ctx_t, do you think?
> > [[[
> > /**
> > * Client context
> > *
> > + * A client should only access a context from one thread at a time, as
> > + * the context includes batons that typically point to mutable data which
> > + * cannot be safely shared across threads.
> > + *
> > * @defgroup clnt_ctx Client context management
> > *
> > * @{
> > */
> > ]]]
>
> It's not clear to me from the added paragraph whether the restriction
> "only access the context from a thread at a time" is imposed by the
> library implementation or by the way API consumers' code is typically
> structured. That is: if a client is careful to use mutexes in his
> batons, or for that matter passes NULL for all batons, would the
> restriction still apply?
- Julian
Re: svn commit: r1834612 - in
/subversion/branches/1.10.x-issue4758/subversion: libsvn_client/shelve.c
tests/cmdline/shelve_tests.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Foad wrote on Mon, 02 Jul 2018 12:20 +0100:
> Philip Martin wrote:
> > I did also worry about thread safety: it's not safe to modify the
> > context like that if the context can be shared across multiple threads.
> > However the context also includes batons and those typically point to
> > mutable data which cannot be shared across threads either. I suppose we
> > should document that a client should only access a context from one
> > thread at a time.
>
> Documenting that sounds reasonable. Like this, just above typedef struct
> svn_client_ctx_t, do you think?
> [[[
> /**
> * Client context
> *
> + * A client should only access a context from one thread at a time, as
> + * the context includes batons that typically point to mutable data which
> + * cannot be safely shared across threads.
> + *
> * @defgroup clnt_ctx Client context management
> *
> * @{
> */
> ]]]
It's not clear to me from the added paragraph whether the restriction
"only access the context from a thread at a time" is imposed by the
library implementation or by the way API consumers' code is typically
structured. That is: if a client is careful to use mutexes in his
batons, or for that matter passes NULL for all batons, would the
restriction still apply?
Cheers,
Daniel