Double-Checked Locking

[QiAnXinCodeSafe]

https://github.com/eclipse/californium/blob/688853649bcee162bd7925a6561a238c85bec1d6/californium-core/src/main/java/org/eclipse/californium/core/CoapClient.java#L331-L334

Double-Checked Locking is widely cited and used as an efficient method for implementing lazy initialization in a multithreaded environment.
Unfortunately, it will not work reliably in a platform independent way when implemented in Java, without additional synchronization.

[boaks]

@sbernard31

Is it OK, if I remove the "double check"? Or do you want to replace that with a more sophisticated solution?

[sbernard31]

Unfortunately, it will not work reliably in a platform independent way when implemented in Java, without additional synchronization.

This is not clear to me at all.

But reading this : https://en.wikipedia.org/wiki/Double-checked_locking#Usage_in_Java
I understand that the current implemented pattern does not work as expected.

Correct ways seems to be :

1. setting secondaryExecutor as volatile attribute
2. removing if (secondaryExecutor == null) outside the synchronized block

As 1. seems to be the recommended way since java 5, so would go for 1.

[boaks]

AFAIK, there have been some discussions over the years, if that pattern should be applied or not.
The wiki-pedia mentions therefore "At times, it can be considered an anti-pattern."

For modern java-vms, it seems to have not too much advantages.
I this case, just for the observes, I also don't see, that this will speed up.
In my opinion a simple synchronized solution will do it.
If you prefer to keep it with volatile, that will be OK as well.

[sbernard31]

For modern java-vms, it seems to have not too much advantages.

I'm curious any Source ?

I this case, just for the observes, I also don't see, that this will speed up.
In my opinion a simple synchronized solution will do it.

I don't know, maybe you're right.

If you prefer to keep it with volatile, that will be OK as well.

Personally, I would just follow the wikipedia page which considers the volatile stuff as the best practice for lazy loading giving explanation and source, but I have no strong opinion.

⚠️ If you add the volatile, do not forget to also add the localRef

Note the local variable "localRef", which seems unnecessary. The effect of this is that in cases where helper is already initialized (i.e., most of the time), the volatile field is only accessed once (due to "return localRef;" instead of "return helper;"), which can improve the method's overall performance by as much as 40 percent.

[boaks]

I'm curious any Source ?

RSPEC-2168

With early versions of the JVM, synchronizing the whole method was generally advised against for performance reasons. But synchronized performance has improved a lot in newer JVMs, so this is now a preferred solution.

[sbernard31]

Interesting.

I try to go deeper in this topic, not to decide what to do for this particular issue but more for my general knowledge.

With early versions of the JVM, synchronizing the whole method was generally advised against for performance reasons. But synchronized performance has improved a lot in newer JVMs, so this is now a preferred solution.

I look at see references of this rule and I see nothing about this, neither what could be early version of the JVM.
I would expect something more precise from sonasource.

I try to find some information about this on the web and for now didn't find so much about that.
Maybe this : http://tutorials.jenkov.com/java-concurrency/synchronized.html#synchronized-block-performance-overhead

Probably not directly linked but I fall on this interesting "news" : https://openjdk.java.net/jeps/374.
Which bring me to : https://www.javaspecialists.eu/archive/Issue282-Biased-locking-a-goner-but-better-things-Loom-ahead.html

(If one day you find more information about that do not hesitate to share)

[boaks]

#1431

I would like to have it fixed for the 2.5.0.

So, @sbernard31,would it be possible for you to provide a fix?
Alternatively, I just remove the "double check".
Or we fix it after 2.5.0.

[sbernard31]

I would like to have it fixed for the 2.5.0.

No objection.

So, @sbernard31,would it be possible for you to provide a fix?

It seems that the current way I implemented double-check is not correct.
Reading several links above it seems the right way would be something like :

/** Scheduled executor intended to be used for rare executing timers (e.g. cleanup tasks). */
private volatile ScheduledThreadPoolExecutor secondaryExecutor;
private volatile boolean detachExecutor;
private ScheduledThreadPoolExecutor getSecondaryExecutor() {
	// This code comes from  :
	// https://en.wikipedia.org/wiki/Double-checked_locking#Usage_in_Java
	// https://www.oracle.com/technical-resources/articles/javase/bloch-effective-08-qa.html
	// AND SHOULD NOT BE MODIFIED
	ScheduledThreadPoolExecutor localRef = secondaryExecutor;
	if (localRef == null) {
		synchronized (this) {
			localRef = secondaryExecutor;
			if (localRef == null) {
				secondaryExecutor = localRef = new ScheduledThreadPoolExecutor(1,
						new NamedThreadFactory("CoapClient(secondary)#"));
			}
			this.detachExecutor = false; 
		}
	}
	return secondaryExecutor;
}

Note :
That detachExecutor attribute is out of the pattern, so we are not sure if code is correct like this...
And I'm not sure at all if this will solve this :

Unfortunately, it will not work reliably in a platform independent way when implemented in Java, without additional synchronization.

As I didn't find any information about that.

Alternatively, we can just remove the double check and keep in mind that we have maybe a performance issue here :

/** Scheduled executor intended to be used for rare executing timers (e.g. cleanup tasks). */
private volatile ScheduledThreadPoolExecutor secondaryExecutor;
private volatile boolean detachExecutor;

private ScheduledThreadPoolExecutor getSecondaryExecutor() {
	// Warning there is maybe a performance issue here, see : 
	// - https://en.wikipedia.org/wiki/Double-checked_locking#Usage_in_Java
	// - https://github.com/eclipse/californium/issues/1420
	synchronized (this) {
		if (secondaryExecutor == null) {
			secondaryExecutor = new ScheduledThreadPoolExecutor(1,
					new NamedThreadFactory("CoapClient(secondary)#"));
			this.detachExecutor = false;
		}
	}
}

I have no strong opinion, but maybe second solution is safer ?

[boaks]

I have no strong opinion, but maybe second solution is safer ?

I would prefer the second.

[sbernard31]

Done. (see #1438)