> -----Original Message-----
> From: Greg Stein [mailto:gstein_at_gmail.com]
> Sent: maandag 11 oktober 2010 11:03
> To: Bert Huijben
> Cc: dev_at_subversion.apache.org; artagnon_at_apache.org
> Subject: Re: svn commit: r1004286 - in /subversion/trunk: ./
> subversion/libsvn_subr/io.c
>
> On Mon, Oct 11, 2010 at 04:58, Bert Huijben <bert_at_qqmail.nl> wrote:
> >> -----Original Message-----
> >> From: artagnon_at_apache.org [mailto:artagnon_at_apache.org]
> >> Sent: maandag 4 oktober 2010 17:27
> >> To: commits_at_subversion.apache.org
> >> Subject: svn commit: r1004286 - in /subversion/trunk: ./
> >> subversion/libsvn_subr/io.c
> >>
> >> Author: artagnon
> >> Date: Mon Oct 4 15:26:44 2010
> >> New Revision: 1004286
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1004286&view=rev
> >> Log:
> >> Merge r985477 from subversion/branches/performance
> >>
> >> * subversion/libsvn_subr/io.c
> >> (get_default_file_perms): Store the permissions of the created
> >> temporary file in a static variable and re-use it in subsequent
> >> calls instead of checking persmissions everytime. This has
> >> performance benefits.
> >>
> >> Review by: artagnon
> >> Approved by: julianfoad
> >
> > Delayed review:
> >
> > Shouldn't this function use some 'atomic initialization' handling?
> >
> > Currently it uses a static apr_fileperms_t (integer?) which can be
> manipulated by multiple threads at the same time.
> >
> > This part of subversion is a library and inside tools like Subclipse,
> TortoiseSVN, AnkhSVN and others it is used multithreaded.
>
> So what? Aren't all of those threads going to write the exact same
> value into the variable?
>
> And if they *don't*, then I believe we have larger problems.
No, they are taking the value that is being stored there at the same time
(read: value is undefined) and use that as the umask.
The pattern
static long x;
if (x == 0)
{
<calculating>
x = <value>
}
<use x>
is not thread safe.
Bert
Received on 2010-10-11 11:11:58 CEST