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

Fixed IllegalAccessExceptions that occurred after JMV garbage collection #104

Conversation

SanderBenschop
Copy link
Member

An error sometimes occurred in the BeanMapper when it attempted to
invoke a getter method in the PropertyDescriptorPropertyAccessor.

The short version of the solution:

The setAccessible method should be called very soon prior to calling
the invoke method rather than only once at the creation of the
PropertyDescriptorPropertyAccessor.

For the long version of the solution, let me take you down the
rabbit hole:

The error occurred in the following piece of code, when the 'invoke'
method was called:

@Override
public Object getValue(Object instance) {
    if (!isReadable()) {
        throw new BeanGetFieldException(instance.getClass(), getName());
    }

    try {
        return descriptor.getReadMethod().invoke(instance);
    } catch (IllegalAccessException e) {
        throw new BeanGetFieldException(instance.getClass(), getName(), e);
    } catch (InvocationTargetException e) {
        throw new BeanGetFieldException(instance.getClass(), getName(), e);
    }
}

This read method was made accessible in the constructor of the
PropertyDescriptorPropertyAccessor:

public PropertyDescriptorPropertyAccessor(PropertyDescriptor descriptor) {
    this.descriptor = descriptor;
    makeAccessable(descriptor.getReadMethod());
    makeAccessable(descriptor.getWriteMethod());
}

private void makeAccessable(Method method) {
    if (method != null) {
        method.setAccessible(true);
    }
}

And the PropertyDescriptorPropertyAccessor is only created once for each
method and afterwards the same instance is used to retrieve the value
from the appropriate method. This can be verified by checking its memory
address.

When the getReadMethod() is called on the descriptor, the same instance
is returned from the memory. Or so it seems.

When you step into the getReadMethod() you can see that it retrieves the
read method from a reference:

Method readMethod = this.readMethodRef.get();

When we step into the get() method on the readMethodRef field,
we see the following body:

 Method get() {
    if (this.methodRef == null) {
        return null;
    }
    Method method = this.methodRef.get();
    if (method == null) {
        method = find(this.typeRef.get(), this.signature);
        if (method == null) {
            this.signature = null;
            this.methodRef = null;
            this.typeRef = null;
        }
        else {
            this.methodRef = new SoftReference<>(method);
        }
    }
    return isPackageAccessible(method.getDeclaringClass()) ? method : null;
}

A SoftReference is created if the current value of methodRef is null,
the next time the same SoftReference instance is returned. On this
SoftReference's referent object (which is an instance of Method)
a setAccessible(true) call is done when the PropertyDescriptorPropertyAccessor
is created.

In the JavaDoc of a SoftReference, the following comment is shown:

 * Soft reference objects, which are cleared at the discretion of the garbage
 * collector in response to memory demand.  Soft references are most often used
 * to implement memory-sensitive caches.

It seems that at some point in time the reference is deleted when the garbage
collector decides it is time. Then, a new reference is created for which the
setAccessible method has NOT been called. This results in an
IllegalAccessException and halts the mapping.

When we call setAccessible just before we call the invoke, we set the accessibility
on the SoftReference we just got back or created. The garbage collector can not clean
this up in the meantime as we still have a reference to it.

The same fix was performed for both field and and method accessors.

@codecov
Copy link

codecov bot commented Mar 6, 2018

Codecov Report

Merging #104 into master will increase coverage by 0.48%.
The diff coverage is 80%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #104      +/-   ##
============================================
+ Coverage     93.95%   94.43%   +0.48%     
  Complexity      764      764              
============================================
  Files            79       79              
  Lines          1604     1600       -4     
  Branches        151      151              
============================================
+ Hits           1507     1511       +4     
+ Misses           58       52       -6     
+ Partials         39       37       -2
Impacted Files Coverage Δ Complexity Δ
...anmapper/core/inspector/FieldPropertyAccessor.java 71.42% <100%> (+1.42%) 15 <2> (ø) ⬇️
.../inspector/PropertyDescriptorPropertyAccessor.java 87.09% <75%> (+20.43%) 17 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87a3d88...8b073ba. Read the comment docs.

An error sometimes occurred in the BeanMapper when it attempted to
invoke a getter method in the PropertyDescriptorPropertyAccessor.

The short version of the solution:

	The setAccessible method should be called very soon prior to calling
	the invoke method rather than only once at the creation of the
	PropertyDescriptorPropertyAccessor.

For the long version of the solution, let me take you down the
rabbit hole:

	The error occurred in the following piece of code, when the 'invoke'
	method was called:

	@OverRide
	public Object getValue(Object instance) {
	    if (!isReadable()) {
	        throw new BeanGetFieldException(instance.getClass(), getName());
	    }

	    try {
	        return descriptor.getReadMethod().invoke(instance);
	    } catch (IllegalAccessException e) {
	        throw new BeanGetFieldException(instance.getClass(), getName(), e);
	    } catch (InvocationTargetException e) {
	        throw new BeanGetFieldException(instance.getClass(), getName(), e);
	    }
	}

	This read method was made accessible in the constructor of the
	PropertyDescriptorPropertyAccessor:

	public PropertyDescriptorPropertyAccessor(PropertyDescriptor descriptor) {
	    this.descriptor = descriptor;
	    makeAccessable(descriptor.getReadMethod());
	    makeAccessable(descriptor.getWriteMethod());
	}

	private void makeAccessable(Method method) {
	    if (method != null) {
	        method.setAccessible(true);
	    }
	}

	And the PropertyDescriptorPropertyAccessor is only created once for each
	method and afterwards the same instance is used to retrieve the value
	from the appropriate method. This can be verified by checking its memory
	address.

	When the getReadMethod() is called on the descriptor, the same instance
	is returned from the memory. Or so it seems.

	When you step into the getReadMethod() you can see that it retrieves the
	read method from a reference:

	Method readMethod = this.readMethodRef.get();

	When we step into the get() method on the readMethodRef field,
	we see the following body:

	 Method get() {
	    if (this.methodRef == null) {
	        return null;
	    }
	    Method method = this.methodRef.get();
	    if (method == null) {
	        method = find(this.typeRef.get(), this.signature);
	        if (method == null) {
	            this.signature = null;
	            this.methodRef = null;
	            this.typeRef = null;
	        }
	        else {
	            this.methodRef = new SoftReference<>(method);
	        }
	    }
	    return isPackageAccessible(method.getDeclaringClass()) ? method : null;
	}

	A SoftReference is created if the current value of methodRef is null,
	the next time the same SoftReference instance is returned. On this
	SoftReference's referent object (which is an instance of Method)
	a setAccessible(true) call is done when the PropertyDescriptorPropertyAccessor
	is created.

	In the JavaDoc of a SoftReference, the following comment is shown:

	 * Soft reference objects, which are cleared at the discretion of the garbage
	 * collector in response to memory demand.  Soft references are most often used
	 * to implement memory-sensitive caches.

	It seems that at some point in time the reference is deleted when the garbage
	collector decides it is time. Then, a new reference is created for which the
	setAccessible method has NOT been called. This results in an
	IllegalAccessException and halts the mapping.

	When we call setAccessible just before we call the invoke, we set the accessibility
	on the SoftReference we just got back or created. The garbage collector can not clean
	this up in the meantime as we still have a reference to it.

	The same fix was performed for both field and and method accessors.
@SanderBenschop SanderBenschop force-pushed the bugfix/illegal-access-exception-garbage-collected-soft-reference branch from b4773bb to 8b073ba Compare March 6, 2018 15:05
@42BV 42BV deleted a comment Mar 6, 2018
@robert-bor robert-bor merged commit 4351129 into master Mar 6, 2018
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

Successfully merging this pull request may close these issues.

None yet

2 participants