The Wayback Machine - https://web.archive.org/web/20210119024200/https://github.com/iluwatar/java-design-patterns/issues/650
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Double-checked locking more antipattern than pattern #650

Open
swhittaker opened this issue Oct 2, 2017 · 8 comments
Open

Double-checked locking more antipattern than pattern #650

swhittaker opened this issue Oct 2, 2017 · 8 comments

Comments

@swhittaker
Copy link

@swhittaker swhittaker commented Oct 2, 2017

While some JVMs may do the expected thing, a memory model following the java specification may fail in multiple ways.

There are some way to make it work, such as using volatile and final field declarations.

The following links contained more detail:
https://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
https://www.javaworld.com/article/2074979/java-concurrency/double-checked-locking--clever--but-broken.html

@pbougrier
Copy link

@pbougrier pbougrier commented Jan 10, 2018

@swhittaker is right. See in his first link, the authors of the declaration (Joshua Bloch, Doug Lea...). They're recognized by the Java community as Gurus, and they tell us the double check pattern is dangerous. Delete the page or put it in a new section called "antipatterns".

@iluwatar
Copy link
Owner

@iluwatar iluwatar commented Jan 10, 2018

Thanks for pointing this out. The articles seem credible and we should follow their advice. This affects Double Checked Locking pattern and one example of Singleton.

We don't have antipatterns section so the correct action would be to delete those examples. I will accept PR that does it.

@swhittaker
Copy link
Author

@swhittaker swhittaker commented Jan 12, 2018

I think it would be a shame to simply remove the examples. As it is a known pattern, and now established anti-pattern, I think it still belongs here, preserved in a new anti-pattern section as suggested by @pbougrier, with an explanation as to why it is not recommended for use.

At the least, a Consequences header could be added to the pattern description (as done in singleton) with the reasons to not use it included, along with an explanation of the circumstances under which the pattern is valid.

@iluwatar
Copy link
Owner

@iluwatar iluwatar commented Jan 12, 2018

Yes, I guess we could do that (explain it should not be used) and add antipattern tag to http://java-design-patterns.com/patterns/ page.

@Auffahrend
Copy link

@Auffahrend Auffahrend commented Jan 12, 2018

Guys, did you read the articles to the end? The Double Checked Locking was considered to be broken before JMM of java 1.5. In 1.5 they changed the semantics of volatile. If you make your "singleton" (or whatever you are double-checking) volatile it works fine.
I would suggest updating the description with notes on using volatile and required version of java.
I could do that, if you don't mind.

@swhittaker
Copy link
Author

@swhittaker swhittaker commented Jan 12, 2018

@Auffahrend please note that the links also state that unless all the fields in the resource being protected are themselves immutable (final) then those fields also need to be declared volatile.

To my mind, even if this works, having this restriction imposed on the fields of a class just to provide lazy loading when constructing it is untenable as it introduces maintenance and performance implications to that class for little gain.

This isn't a case of "if we can" and more a case of "if we should".

@Auffahrend
Copy link

@Auffahrend Auffahrend commented Jan 12, 2018

@swhittaker well, in case of "if we should" clearly describing pros and cons and requirements of pattern's implementation becomes even more important.
Also please note, that is not just lazy loading when this pattern is usable. We use it in cache engine of web service, double checking presence of a cached value. It prevents concurrent web requests of duplicating DB requests or perform duplicate calculations.

@swhittaker
Copy link
Author

@swhittaker swhittaker commented Jan 12, 2018

Ok, so with a bit more research I've found this stackoverflow post where the accepted answer has an example for how to safely use DCL when in use for lazy loading. It seems like this may no longer be an anti-pattern (but the way in which it is often incorrectly implemented is and is worth a mention).

The entry should be updated to note that there are better alternatives to DCL when working with a static resource, and to include the previously mentioned points around final fields / volatile fields.

This is a good resource with multiple working and non-working examples of initialisation including DCL.

However this is all to do with constructing an object, I've not seen a safe example of DCL where the value can be changed again after being set the first time (as in caching).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.