> -----Original Message-----
> From: Julian Foad [mailto:julian.foad_at_wandisco.com]
> Sent: maandag 11 oktober 2010 11:46
> To: Bert Huijben
> Cc: 'Greg Stein'; dev_at_subversion.apache.org; artagnon_at_apache.org
> Subject: RE: svn commit: r1004286 - in /subversion/trunk: ./
> subversion/libsvn_subr/io.c
>
> On Mon, 2010-10-11, Julian Foad wrote:
> > On Mon, 2010-10-11, Bert Huijben wrote:
> > > > -----Original Message-----
> > > > From: Greg Stein [mailto:gstein_at_gmail.com]
> > > > 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]
> > > > >> * 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.
> > > > >
> > > > > 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.
>
> > <http://stackoverflow.com/questions/54188/are-c-reads-and-writes-of-
> an-int-atomic>.
> > "On an IA32 a correctly aligned address will be an atomic operation.
> > [... otherwise... can't assume it is]."
>
> Sorry, I pressed "Send" too early. That's not the most important bit of
> information. (That paragraph talks mostly about <= 32-bit CPUs, where
> of course there will be problems.) Bert explained to me on IRC that
> atomicity is not guaranteed even on >= 32 bit architectures, and the
> highest-ranked answer on that web page agrees. I'm no expert in this so
> I'll go away now.
Let me add that calling apr_atomic_set32() instead of the 'x = <value>' part of the pattern will fix this issue in the way that was documented by the comment in the code: all threads do the same thing and one of the results is left in the static variable.
Another option is to use an atomic initialization function to initialize the value just once.
Bert
Received on 2010-10-11 11:51:35 CEST