jackrabbit-users mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marc Speck" <marcsp...@gmail.com>
Subject Re: Extending SimpleWebDavServlet
Date Mon, 29 Sep 2008 06:55:23 GMT
Thanks for the replies, Alex and Felix.



> The original author is
> currently on vacation, maybe she can answer you later.
>
Sure, I've just done the same.

Before giving my reply, I want to emphasize that I'm on thin ice when
talking about memory model in Java 1.5 let alone 1.4 that Jackrabbit
requires. Multi-threading in Java is next to black magic to me, so I rather
cite below the sources of information.



> > public DavLocatorFactory getLocatorFactory() {
> >     if (locatorFactory == null) {
> >         locatorFactory = new LocatorFactoryImplEx(resourcePathPrefix);
> >     }
> >     return locatorFactory;
> > }
> >
> >
> > I just ask why this is thread safe. It is not called in servlet.init() or
> > the like, isn't it?
>
> I assume the default implementation has no thread issues actually
> because the LocatorFactoryImplEx is a very simple class with a single
> internal field set by the constructor.


Actually, this internal field is the source of a potential data race: thread
A runs up to the point, when it has constructed LocatorFactoryImplEx but has
not yet assigned the internal field. Thread A gets on hold relative to
thread B. B runs through the getter and sees that locatorFactory is not
null, fetches the object, access the uninitialized internal field and voilĂ !
We have a mess.

What's generally confusing to me is that you never know about the variable
visibility between threads and the relative succession of different threads
(sometimes called "happens-before"). The lazy initialization problem as
shown above is very well described in "Java concurency in practice" by Brian
Goetz under "16.2.1 Unsafe publication".


>
> In case of multi-threaded call to the getLocatorFactory method, multiple
> instances of the LocatorFactoryImplEx class may be created of which only
> a single one will ultimately remain assigned to the locatorFactory field.


Yes, that's not a problem and can also happen with the suggestion below.



>
> If you overwrite this method and have a more complicated implementation
> of the DavLocatorFactory interface, which you want to ensure, that only
> a single instance is created, I suggest you implement synchronization in
> your overwritten method.


Agreed but this is not the issue.

If there is a data race present, other methods like
SimpleWebdavServlet.getResourceFactory() show the same behavior.

The best solution by far is not using lazy initialization at all. If you
still want to delay the initialization time (and spend finally more time
with all the getters), you could use the single-check idiom as described
e.g. in Effective Java by Joshua Bloch, 2nd edition, p. 284. This works in
Java 1.5, I think there is a problem with volatile in 1.4.

private volatile DavLocatorFactory locatorFactory
public DavLocatorFactory getLocatorFactory() {
   DavLocatorFactory result = locatorFactory
   if (result == null)
      locatorFactory = result = new
LocatorFactoryImplEx(resourcePathPrefix);
   return result
}

Synchronizing the getter helps also. This is done by some getters in
SimpleWebdavServlet.

I'm happy to learn what's wrong with my line of thoughts about
multi-threading...
Marc

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message