Clojure

Reflection on internal classes fails under Java 9

Details

  • Patch:
    Code and Test
  • Approval:
    Incomplete

Description

Due to changes in reflective access for the Jigsaw module system in Java 9, the Reflector will now fail on some cases that worked in previous Java versions.

(def fac (javax.xml.stream.XMLInputFactory/newInstance))
(.createXMLStreamReader fac (java.io.StringReader. ""))

Here fac will be an instance of com.sun.xml.internal.stream.XMLInputFactoryImpl, which is an implementation of javax.xml.stream.XMLInputFactory. In the new java.xml module, javax.xml.stream is an exported package, but the XMLInputFactoryImpl is an internal implementation of the public interface in that package. The invocation of createXMLStreamReader will be reflective and the Reflector will attempt to invoke the method based on the implementation class, which is not accessible outside the module, yielding:

IllegalAccessException class clojure.lang.Reflector cannot access class com.sun.xml.internal.stream.XMLInputFactoryImpl (in module java.xml) because module java.xml does not export com.sun.xml.internal.stream to unnamed module @4722ef0c
	jdk.internal.reflect.Reflection.throwIllegalAccessException (Reflection.java:423)
	jdk.internal.reflect.Reflection.throwIllegalAccessException (Reflection.java:414)
	jdk.internal.reflect.Reflection.ensureMemberAccess (Reflection.java:112)
	java.lang.reflect.AccessibleObject.slowCheckMemberAccess (AccessibleObject.java:632)
	java.lang.reflect.AccessibleObject.checkAccess (AccessibleObject.java:624)
	java.lang.reflect.Method.invoke (Method.java:539)
	clojure.lang.Reflector.invokeMatchingMethod (Reflector.java:93)
	clojure.lang.Reflector.invokeInstanceMethod (Reflector.java:28)

One workaround here is to avoid the reflective call by type-hinting to the public exported interface:

(.createXMLStreamReader ^javax.xml.stream.XMLInputFactory fac (java.io.StringReader. ""))

Another (undesirable) workaround is to export the private package from java.xml to the unnamed module (which is the module used when code is loaded from the classpath rather than from a module) when invoking java/javac:

java --add-exports=java.xml/com.sun.xml.internal.stream=ALL-UNNAMED --add-exports=java.xml/com.sun.xml.internal.stream.writers=ALL-UNNAMED --add-exports=java.xml/com.sun.org.apache.xerces.internal.impl=ALL-UNNAMED ...

Proposed: The attached patch will check for and catch IllegalAccessException in the Reflector. When it occurs, the Reflector will attempt to invoke the method on all super-classes and super-interfaces in case one of them can succeed.

Patch: tcrawley.CLJ-2066.2017-02-14.patch

More info:

Activity

Hide
Toby Crawley added a comment -
Show
Toby Crawley added a comment - This is the root cause of http://dev.clojure.org/jira/browse/DXML-32
Alex Miller made changes -
Field Original Value New Value
Approval Triaged [ 10120 ]
Affects Version/s Release 1.8 [ 10254 ]
Labels interop
Alex Miller made changes -
Labels interop interop reflection
Hide
Toby Crawley added a comment -

I don't see any indication that Java 9 will change in our favor, so I'm attaching a patch to fix this in Clojure itself.

Show
Toby Crawley added a comment - I don't see any indication that Java 9 will change in our favor, so I'm attaching a patch to fix this in Clojure itself.
Toby Crawley made changes -
Attachment tcrawley.CLJ-2066.2017-02-14.patch [ 16436 ]
Hide
Alex Miller added a comment -

Screening comments:

I think by just inspecting the patch that the new code will attempt to invoke the method on all super classes and interfaces, many of which will not have the applicable method and will thus throw IllegalArgumentException, possibly before finding the correct one. Can we reduce the effort here by only attempting the call on the super types that actually have the method? It would be good to expand the tests to also include this case if possible if I'm just mis-reading things (I did not attempt to construct this scenario).

Also, is there a method we can call to determine whether the method is accessible before invoking and needing to catch IllegalAccessException? (Obviously, we would want something that is not new in Java 9 so that this call worked on older JDKs too.)

Show
Alex Miller added a comment - Screening comments: I think by just inspecting the patch that the new code will attempt to invoke the method on all super classes and interfaces, many of which will not have the applicable method and will thus throw IllegalArgumentException, possibly before finding the correct one. Can we reduce the effort here by only attempting the call on the super types that actually have the method? It would be good to expand the tests to also include this case if possible if I'm just mis-reading things (I did not attempt to construct this scenario). Also, is there a method we can call to determine whether the method is accessible before invoking and needing to catch IllegalAccessException? (Obviously, we would want something that is not new in Java 9 so that this call worked on older JDKs too.)
Alex Miller made changes -
Patch Code and Test [ 10002 ]
Approval Triaged [ 10120 ] Incomplete [ 10006 ]
Description With the module system (jigsaw) as it is currently implemented in Java 9 early access builds (9-ea+144), calling a method via reflection is only allowed if the {{Method}} was retrieved for a class/interface in a package that is exported by its containing module. {{Reflector.java}} currently uses only {{target.getClass()}} to locate the {{Method}}, so reflective method invocation on a non-exported class will fail even if the method is provided by an exported parent interface or superclass.

The current workaround is to export the package to the unnamed module (where an application that doesn't explicitly use the module system runs) when invoking java/javac:

{noformat}
java --add-exports=java.xml/com.sun.xml.internal.stream=ALL-UNNAMED --add-exports=java.xml/com.sun.xml.internal.stream.writers=ALL-UNNAMED --add-exports=java.xml/com.sun.org.apache.xerces.internal.impl=ALL-UNNAMED ...
{noformat}

It's possible that this will be addressed in jigsaw before the release of Java 9. If not, {{Reflector.java}} could be modified to walk the ancestor chain if the initial invocation fails. Note that even with that change, accessing methods that are only defined on the non-exported class (i.e. methods that don't override a method from an exported superclass/interface) will require an {{--add-exports}} option.

For more details, see https://groups.google.com/d/msg/clojure-dev/Tp_WuEEcdWI/LMMQVAUYBwAJ
Due to changes in reflective access for the Jigsaw module system in Java 9, the Reflector will now fail on some cases that worked in previous Java versions.

{code}
(def fac (javax.xml.stream.XMLInputFactory/newInstance))
(.createXMLStreamReader fac (java.io.StringReader. ""))
{code}

Here {{fac}} will be an instance of {{com.sun.xml.internal.stream.XMLInputFactoryImpl}}, which is an implementation of {{javax.xml.stream.XMLInputFactory}}. In the new {{java.xml}} module, {{javax.xml.stream}} is an exported package, but the {{XMLInputFactoryImpl}} is an internal implementation of the public interface in that package. The invocation of {{createXMLStreamReader}} will be reflective and the {{Reflector}} will attempt to invoke the method based on the implementation class, which is not accessible outside the module, yielding:

{code}
IllegalAccessException class clojure.lang.Reflector cannot access class com.sun.xml.internal.stream.XMLInputFactoryImpl (in module java.xml) because module java.xml does not export com.sun.xml.internal.stream to unnamed module @4722ef0c
jdk.internal.reflect.Reflection.throwIllegalAccessException (Reflection.java:423)
jdk.internal.reflect.Reflection.throwIllegalAccessException (Reflection.java:414)
jdk.internal.reflect.Reflection.ensureMemberAccess (Reflection.java:112)
java.lang.reflect.AccessibleObject.slowCheckMemberAccess (AccessibleObject.java:632)
java.lang.reflect.AccessibleObject.checkAccess (AccessibleObject.java:624)
java.lang.reflect.Method.invoke (Method.java:539)
clojure.lang.Reflector.invokeMatchingMethod (Reflector.java:93)
clojure.lang.Reflector.invokeInstanceMethod (Reflector.java:28)
{code}

One workaround here is to avoid the reflective call by type-hinting to the public exported interface:

{code}
(.createXMLStreamReader ^javax.xml.stream.XMLInputFactory fac (java.io.StringReader. ""))
{code}

Another (undesirable) workaround is to export the private package from java.xml to the unnamed module (which is the module used when code is loaded from the classpath rather than from a module) when invoking java/javac:

{noformat}
java --add-exports=java.xml/com.sun.xml.internal.stream=ALL-UNNAMED --add-exports=java.xml/com.sun.xml.internal.stream.writers=ALL-UNNAMED --add-exports=java.xml/com.sun.org.apache.xerces.internal.impl=ALL-UNNAMED ...
{noformat}

*Proposed:* The attached patch will check for and catch IllegalAccessException in the Reflector. When it occurs, the Reflector will attempt to invoke the method on all super-classes and super-interfaces in case one of them can succeed.

*Patch:* tcrawley.CLJ-2066.2017-02-14.patch

*More info:*

- For more details, see https://groups.google.com/d/msg/clojure-dev/Tp_WuEEcdWI/LMMQVAUYBwAJ
- For reference info, see http://openjdk.java.net/projects/jigsaw/spec/sotms/#reflective-readability
Fix Version/s Release 1.9 [ 10750 ]
Labels interop reflection interop reflection regression
Hide
Herwig Hochleitner added a comment -

This may be a stupid question, but has there been any research, whether [1] and [2] could be utilized to handle this without catching exceptions?

[1] http://download.java.net/java/jdk9/docs/api/java/lang/Class.html#getModule--
[2] http://download.java.net/java/jdk9/docs/api/java/lang/reflect/Module.html#isExported-java.lang.String-java.lang.reflect.Module-

Show
Herwig Hochleitner added a comment - This may be a stupid question, but has there been any research, whether [1] and [2] could be utilized to handle this without catching exceptions? [1] http://download.java.net/java/jdk9/docs/api/java/lang/Class.html#getModule-- [2] http://download.java.net/java/jdk9/docs/api/java/lang/reflect/Module.html#isExported-java.lang.String-java.lang.reflect.Module-
Hide
Alex Miller added a comment -

Nope, that's the kind of thing I was asking about in my comment. However, those are jdk 9 only methods and right now Clojure supports jdk 6+. That's not impossible to deal with but does increase the difficulty.

Show
Alex Miller added a comment - Nope, that's the kind of thing I was asking about in my comment. However, those are jdk 9 only methods and right now Clojure supports jdk 6+. That's not impossible to deal with but does increase the difficulty.
Hide
Toby Crawley added a comment -

Alex: you are correct, an intermediate ancestor that does not provide the method will cause the lookup to fail - I'll rework and provide a new patch with better tests.

Herwig: we could build Clojure as a multi-release jar in order to have code that can use Java 9 features, but that would definitely complicate the build. However, there may be other Java 9 issues that may require us to take that option (I'm investigating one potential issue now).

Show
Toby Crawley added a comment - Alex: you are correct, an intermediate ancestor that does not provide the method will cause the lookup to fail - I'll rework and provide a new patch with better tests. Herwig: we could build Clojure as a multi-release jar in order to have code that can use Java 9 features, but that would definitely complicate the build. However, there may be other Java 9 issues that may require us to take that option (I'm investigating one potential issue now).
Hide
Alex Miller added a comment -

Yeah, I was looking at the multi release jar stuff yesterday. I would really like to avoid that if at all possible as it adds a bunch of work for build, ci, and test.

Show
Alex Miller added a comment - Yeah, I was looking at the multi release jar stuff yesterday. I would really like to avoid that if at all possible as it adds a bunch of work for build, ci, and test.
Toby Crawley made changes -
Attachment tcrawley.CLJ-2066.2017-03-13.patch [ 16545 ]
Toby Crawley made changes -
Attachment tcrawley.CLJ-2066.2017-02-14.patch [ 16436 ]
Hide
Toby Crawley added a comment -

I've attached a new patch (tcrawley.CLJ-2066.2017-03-13.patch) and removed the old one (tcrawley.CLJ-2066.2017-02-14.patch). The new patch checks for any matching methods before calling invokeMatchingMethod; this will prevent the lack of the method on an ancestor from throwing IllegalArgumentException and aborting the lookup process.

I wasn't able to provide a test for the case where the method was missing on an ancestor, since that requires trying to invoke the method through an interface that doesn't provide it (all subclasses will provide methods from parent classes, so you need an ancestor tree that pulls in an interface), and I couldn't find anything in Java's stdlib that did that and was a non-exposed class. To implement a proper test for this, I believe we'd need to construct a module ourselves, which would require changes to the build process to build that module (and run the test) only under Java 9. I did, however, confirm locally that the old code was broken in this regard, and that the new patch fixes it.

Show
Toby Crawley added a comment - I've attached a new patch (tcrawley.CLJ-2066.2017-03-13.patch) and removed the old one (tcrawley.CLJ-2066.2017-02-14.patch). The new patch checks for any matching methods before calling invokeMatchingMethod; this will prevent the lack of the method on an ancestor from throwing IllegalArgumentException and aborting the lookup process. I wasn't able to provide a test for the case where the method was missing on an ancestor, since that requires trying to invoke the method through an interface that doesn't provide it (all subclasses will provide methods from parent classes, so you need an ancestor tree that pulls in an interface), and I couldn't find anything in Java's stdlib that did that and was a non-exposed class. To implement a proper test for this, I believe we'd need to construct a module ourselves, which would require changes to the build process to build that module (and run the test) only under Java 9. I did, however, confirm locally that the old code was broken in this regard, and that the new patch fixes it.
Hide
Toby Crawley added a comment -

After reviewing tcrawley.CLJ-2066.2017-03-13.patch, I can see a new issue with it: if the method being invoked exists only on the non-public class, then the patch will throw an IllegalArgumentException claiming that the method doesn't exist instead of the original, more accurate IllegalAccessException. I'll rework and provide another patch.

Show
Toby Crawley added a comment - After reviewing tcrawley.CLJ-2066.2017-03-13.patch, I can see a new issue with it: if the method being invoked exists only on the non-public class, then the patch will throw an IllegalArgumentException claiming that the method doesn't exist instead of the original, more accurate IllegalAccessException. I'll rework and provide another patch.
Toby Crawley made changes -
Attachment tcrawley.CLJ-2066.2017-03-16.patch [ 16551 ]
Toby Crawley made changes -
Attachment tcrawley.CLJ-2066.2017-03-13.patch [ 16545 ]
Hide
Toby Crawley added a comment -

New patch attached (tcrawley.CLJ-2066.2017-03-16.patch) that will properly throw the original IllegalAccessException when the method only exists on the non-public class. Patch tcrawley.CLJ-2066.2017-03-13.patch has been deleted.

Show
Toby Crawley added a comment - New patch attached (tcrawley.CLJ-2066.2017-03-16.patch) that will properly throw the original IllegalAccessException when the method only exists on the non-public class. Patch tcrawley.CLJ-2066.2017-03-13.patch has been deleted.

People

Vote (1)
Watch (6)

Dates

  • Created:
    Updated: