You are viewing a plain text version of this content. The canonical link for it is here.
Posted to users@jackrabbit.apache.org by Marc Speck <ma...@gmail.com> on 2008/09/02 08:55:37 UTC
Extending SimpleWebDavServlet
While extending SimpleWebDavServlet, I could not see at first glance
how getLocatorFactory()
(
http://fisheye6.atlassian.com/browse/jackrabbit/trunk/jackrabbit-jcr-server/src/main/java/org/apache/jackrabbit/webdav/simple/SimpleWebdavServlet.java?r=603178#l204)
is called the first time (for lazy initialization) in a thread-safe way.
Thanks for any hints,
Marc
Re: Extending SimpleWebDavServlet
Posted by Marc Speck <ma...@gmail.com>.
Very interesting, thanks for the feedback! Let me see whether I got it:
1. All statements below refer to Java 1.5.
2. fieldX = new SomeType(zzz)
- This is thread-safe if SomeType is immutable. An immutable object
features:
- unmodifiable state
- all fields are final and all fields hold immutable objects
- proper construction: "this" is not escaped in the constructor.
- LocatorFactoryImplEx is an immutable object
3. Construction and assigning different instances of LocatorFactoryImplEx()
to locatorFactory is allowed
4. Running Jackrabbit under Java 1.4 has got threading issues because of the
revised final as mentioned above.
5. > 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.
This is wrong for immutable objects like LocatorFactoryImplEx(). It only
applies to mutable objects.
Can I suggest to add a note in the doc that all those classes need to be
immutable? Next, I gonna learn why the mutable resourceConfig in
SimpleWebDavServlet.getResourceFactory() is fine for multiple threads...
Marc
Re: Extending SimpleWebDavServlet
Posted by Marcel Reutegger <ma...@gmx.net>.
Connor, Brett (LNG-TWY) wrote:
> But there a situation where one thread can be using one locator factory and
> another thread using a different one isn't there?
correct, but AFAIU that's not an issue in this case. the locator factory does
not have to be a singleton in a strict sense.
regards
marcel
RE: Extending SimpleWebDavServlet
Posted by "Connor, Brett (LNG-TWY)" <Br...@lexisnexis.co.uk>.
Marcel wrote:
>
> Marc Speck wrote:
> > 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.
>
> the revised memory model in java 1.5 guarantees that final fields are properly assigned after the constructor finishes. other threads accessing the constructed locator factory instance will always see the correct field value.
>
> see: http://www.cs.umd.edu/~pugh/java/memoryModel/jsr-133-faq.html#finalWrong
That's true, if there's only final fields involved. The setting the reference in the getter is atomic so at worst there will be duplicate factories for a while.
But there a situation where one thread can be using one locator factory and another thread using a different one isn't there? May not be a problem in the example of a simple factory but can't be good in more complex examples.
>
> regards
> marcel
REED ELSEVIER (UK) LIMITED - Registered office - 1-3 STRAND, LONDON WC2N 5JR
Registered in England - Company No. 02746621
Re: Extending SimpleWebDavServlet
Posted by Marcel Reutegger <ma...@gmx.net>.
Marc Speck wrote:
> 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.
the revised memory model in java 1.5 guarantees that final fields are properly
assigned after the constructor finishes. other threads accessing the constructed
locator factory instance will always see the correct field value.
see: http://www.cs.umd.edu/~pugh/java/memoryModel/jsr-133-faq.html#finalWrong
regards
marcel
RE: Extending SimpleWebDavServlet
Posted by "Connor, Brett (LNG-TWY)" <Br...@lexisnexis.co.uk>.
Marc wrote:
>
> Hi Felix
>
> > I am neither an export on any Memory Model issue, but I always
thought
> > that the construct
> >
> > fieldX = new SomeType(zzz);
> >
> > would be ok, because fieldX would only be set after the SomeType
> > instance has properly been setup and ready for use.
>
>
> I would have thought the same if I had not learned it differently...
again, there is some black magic in the
> Java air ;) I do not understand this mechanism in the memory model but
I would be thankful if someone could provide an explanation.
The statement you make above only holds true within the single thread
running this code. The issue is updating state between private thread
memory in the JVM. One of the things that 'synchronized' does as well as
the more obvious acquiring the object monitor, it provides what they
call a memory check or memory gate or something - a signal to the VM to
sync changes in this thread with the global memory space. Without this
there are no guarantees that the updates are in the write order AFA
other threads see them. For example the reference to the object may be
visible but the object itself may not be initialized.
Google for "double check locking unsafe" I think you'll find some better
descriptions.
"Volatile" helps but as was said earlier it was broken until recent VMs.
Have a read of JSR 133 (I think) - it's a bit of a mind bend!
> > Have you encountered a concrete problem with this or is a pure
> > academic discussion ?
>
>
>
> This is pure academic, but I crunch multi-threading bugs rather on
this level than tracking bugs that just happen "now and then".
I agree. I've seen issues with Jackrabbit clustering for example that
may be down to this kind of thing but very hard to track down.
Brett
REED ELSEVIER (UK) LIMITED - Registered office - 1-3 STRAND, LONDON WC2N 5JR
Registered in England - Company No. 02746621
Re: Extending SimpleWebDavServlet
Posted by Marc Speck <ma...@gmail.com>.
Hi Felix
> I am neither an export on any Memory Model issue, but I always thought
> that the construct
>
> fieldX = new SomeType(zzz);
>
> would be ok, because fieldX would only be set after the SomeType
> instance has properly been setup and ready for use.
I would have thought the same if I had not learned it differently... again,
there is some black magic in the Java air ;)
I do not understand this mechanism in the memory model but I would be
thankful if someone could provide an explanation. The only thing I did is a
pattern matching between the lazy initialization in SimpleWebDavServlet and
the two books mentioned above. Both state that this implementation is not
thread safe.
>
>
> Have you encountered a concrete problem with this or is a pure academic
> discussion ?
This is pure academic, but I crunch multi-threading bugs rather on this
level than tracking bugs that just happen "now and then".
Marc
Re: Extending SimpleWebDavServlet
Posted by Felix Meschberger <fm...@gmail.com>.
Hi Marc,
You are getting me on my left foot ;-)
I am neither an export on any Memory Model issue, but I always thought
that the construct
fieldX = new SomeType(zzz);
would be ok, because fieldX would only be set after the SomeType
instance has properly been setup and ready for use.
Have you encountered a concrete problem with this or is a pure academic
discussion ?
Regards
Felix
Marc Speck schrieb:
> 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
>
Re: Extending SimpleWebDavServlet
Posted by Marc Speck <ma...@gmail.com>.
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
Re: Extending SimpleWebDavServlet
Posted by Felix Meschberger <fm...@gmail.com>.
Hi Marc,
Marc Speck schrieb:
> Ok, everybody seems to silently agree that there is no threading issue with
> this servlet method:
>
> 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.
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.
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.
Regards
Felix
Re: Extending SimpleWebDavServlet
Posted by Alexander Klimetschek <ak...@day.com>.
On Fri, Sep 5, 2008 at 3:08 PM, Marc Speck <ma...@gmail.com> wrote:
> Ok, everybody seems to silently agree that there is no threading issue with
> this servlet method:
I don't think everybody silently agrees, it's just that not everybody
knows the details of the webdav implementation. The original author is
currently on vacation, maybe she can answer you later.
Anyway, have you seen any explicit problems with this method?
Regards,
Alex
>
> 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?
> Thanks,
> Marc
>
>
>
>
> On Tue, Sep 2, 2008 at 8:55 AM, Marc Speck <ma...@gmail.com> wrote:
>>
>> While extending SimpleWebDavServlet, I could not see at first glance how
> getLocatorFactory() (
> http://fisheye6.atlassian.com/browse/jackrabbit/trunk/jackrabbit-jcr-server/src/main/java/org/apache/jackrabbit/webdav/simple/SimpleWebdavServlet.java?r=603178#l204)
> is called the first time (for lazy initialization) in a thread-safe way.
>>
>> Thanks for any hints,
>> Marc
>
--
Alexander Klimetschek
alexander.klimetschek@day.com
Re: Extending SimpleWebDavServlet
Posted by Marc Speck <ma...@gmail.com>.
Ok, everybody seems to silently agree that there is no threading issue with
this servlet method:
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?
Thanks,
Marc
On Tue, Sep 2, 2008 at 8:55 AM, Marc Speck <ma...@gmail.com> wrote:
>
> While extending SimpleWebDavServlet, I could not see at first glance how
getLocatorFactory() (
http://fisheye6.atlassian.com/browse/jackrabbit/trunk/jackrabbit-jcr-server/src/main/java/org/apache/jackrabbit/webdav/simple/SimpleWebdavServlet.java?r=603178#l204)
is called the first time (for lazy initialization) in a thread-safe way.
>
> Thanks for any hints,
> Marc