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-check idiom for lazy initialization of instance fields. Broken example. #8

Open
panph opened this issue Aug 10, 2019 · 2 comments

Comments

@panph
Copy link

panph commented Aug 10, 2019

CHAPTER 11 CONCURRENCY
Item 83: Use lazy initialization judiciously

// Double-check idiom for lazy initialization of instance fields
private FieldType getField() {
	FieldType result = field; // (1)
	if (result == null) { // First check (no locking)
		synchronized (this) {
			if (field == null) // Second check (with locking)
				field = result = computeFieldValue(); // (2)
		}
	}
	return result;
}

This code is broken, because when one thread enters the line (2), other threads may have already read the field into result (line (1)), and it is null, so, when they enter the line (2), they see that field isn't null anymore, because it was already initialized by the first thread so, the method called by those non-first threads, when they start almost immediately after the first thread, returns null in spite that the field was initialized.

To fix this we need to reassign result to the field value again.

Here is a solution that works:

private FieldType getField() {
	FieldType result = field;
	if (result == null) { // First check (no locking)
		synchronized (this) {
			if (field == null) // Second check (with locking)
				field = result = computeFieldValue();
			else
				result = field;
		}
	}
	return result;
}

Or this one, which I like even more, because we check if result is not null first, which happens more often, or to be more precise, always after first initialization.

private FieldType getField() {
	FieldType result = field;
	if (result != null)
		return result;
	synchronized (this) {
		result = field;
		return result != null ? result : (field = computeFieldValue());
	}
}

Attached code demonstrates the error and how the fixes work.

I'm writing this not to accuse (everybody can mistake) Joshua Bloch, whom I respect much and find his book very useful, but to suggest to programmers to use copy-paste technique judiciously.

Thank you.

Lazy.zip

@jbloch
Copy link
Owner

jbloch commented Aug 13, 2019

Hi Alexey. Ouch! I fixed this bug in the second printing of the book (February 2018). I had correct code in the first two editions of the book, and messed it up in the first printing of the the third edition. I publicly apologized and widely distributed the corrected code. You can also see the corrected code on the errata page. When I put the code examples on the web, though, I ran out of time and never got around to updating the code from the later chapters of the book. Thank you so much for bringing this to my attention.

I will fix this bug (in /src/effectivejava/chapter11/item83/Initialization.java) by tomorrow, but I will leave this issue open until I've updated all of the chapters that need it (at a minimum, the Github code for chapters 9-12 requires a once-over).

I apologize that I allowed this known bad code to exist in Github for over a year. I'm well aware of the responsibility that accompanies writing code that will be widely copy-pasted. I aim to illustrate best practices so that you can copy-paste my code without fear. In this case, I failed.

@jbloch
Copy link
Owner

jbloch commented Sep 12, 2019

OK, I finally fixed the broken code. Sorry for the delay. I sincerely hope to find the time to go over the Github code for chapters 9-12 in the very near future (as promised), at which time I'll close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants