Bug on @Getter(lazy=true) and transient fields #1236

Closed
manuel-hegner opened this Issue Nov 17, 2016 · 4 comments

Projects

None yet

4 participants

@manuel-hegner

Hey,
there is a problem that occurs if you have a transient field that is laziely initialized. The generated getter method crashes after deserialization because the field is null. The expected behaviour would be to call the lazy creationmethod again.

So instead of generating something like:

public double[] getCached() {
	java.lang.Object value = this.cached.get();
	if (value == null) {
		synchronized(this.cached) {
			value = this.cached.get();
			if (value == null) {
				final double[] actualValue = expensive();
				value = actualValue == null ? this.cached : actualValue;
				this.cached.set(value);
			}
		}
	}
	return (double[])(value == this.cached ? null : value);
} 

It should be something like this, at least for transient values:

public double[] getCached() {
	if(cached==null) {
		synchronized(this) {
			if(cached==null)
				cached = new java.util.concurrent.AtomicReference<java.lang.Object>();
		}
	}
	java.lang.Object value = this.cached.get();
	if (value == null) {
		synchronized(this.cached) {
			value = this.cached.get();
			if (value == null) {
				final double[] actualValue = expensive();
				value = actualValue == null ? this.cached : actualValue;
				this.cached.set(value);
			}
		}
	}
	return (double[])(value == this.cached ? null : value);
} 
@bulgakovalexander
Contributor

cached = new java.util.concurrent.AtomicReference<java.lang.Object>();
It does not compile because the field is final.

@manuel-hegner

Oh yeah. I forgot that the field is final. So to prevent the serialization bug you could either make it not final or you have to create a totally different way to serialize/deserialize this.

Another way would be to add an error if a lazy getter field is transient.

@Maaartinus
Contributor

I'm afraid the field must be final in order to be visible in other threads (I guess, it could be volatile instead).

Serialization is a dirty beast and AFAIK uses some kind of reflection for setting final fields. This is probably what also lombok should do for transient fields. I believe that there are no visibility issues (as reflection emits the memory barrier needed).

A compile-time error is IMHO a good step before a better solution gets implemented.

@rspilker rspilker closed this in 8ce09de Dec 5, 2016
@rspilker
Collaborator
rspilker commented Dec 5, 2016

Using reflection in a readResolve method that also should be generated if it's not already there, or added to an existing readResolve is for us way too much effort for use case that we didn't encounter a lot. So we chose to give a compile time error instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment