Double-checked locking idiom in Node.java

[genman]

This is basically broken for JVM 1.4 and earlier, although declaring children as volatile may fix it for most 1.4 JVM.

protected Map children() {
 if (children == null) {
 synchronized(this) {
 // check again to see if children is null
 // to avoid possible race condition of another
 // thread having just exited this synchronized() section
 // that initialized children
 //
 if (children == null) {
 if (getFqn().isRoot()) {
 children = new ConcurrentReaderHashMap(64);
 } else {
 children = new ConcurrentReaderHashMap(4);
 }
 }
 }
 }
 return children;
 }

Suggested solution:

protected synchronized Map children() {
 if (children == null) {
 if (getFqn().isRoot()) {
 children = new ConcurrentReaderHashMap(64);
 } else {
 children = new ConcurrentReaderHashMap(4);
 }
 }
 return children;
 }

Comments?

[anil.saldhana]

Synchronizing on the method seems right. Isn;t that what is done with the classic singleton?

[genman]

I'd like to fix this for the next release, any objections?

[manik]

I'm ok with this

[ben.wang]

I am ok but curious about the performance impact.

In addition, if you trace the usage, it is used eventuall in createNode() of which seems to be called from CacheLoaderInterceptor and CreateIfNotExistsInterceptor.

But for CreateIfNoteExistsInterceptor, we have "synchronized(this)" block there already, so that kind of makes this moot. However, in CacheLoaderInterceptor, there is no such sync block. So maybe we want to clean it up and think of the semantics in both places.

[belaban]

We don't use the CreateIfNotExistsInterceptor anymore, maybe this class should be removed...