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
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.
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.)
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.
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.
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.
Hide
Ghadi Shayban added a comment -

In JDK9 public classes housed in a non-exported package are now inaccessible. Previously public classes might not be public any more. Clojure should link to them neither non-reflectively nor reflectively – perhaps a slightly larger adjustment to the compiler/reflector method lookup?

Show
Ghadi Shayban added a comment - In JDK9 public classes housed in a non-exported package are now inaccessible. Previously public classes might not be public any more. Clojure should link to them neither non-reflectively nor reflectively – perhaps a slightly larger adjustment to the compiler/reflector method lookup?
Hide
Herwig Hochleitner added a comment -

Ghadi: does that mean, that we can't even get at the Class object to call http://download.java.net/java/jdk9/docs/api/java/lang/Class.html#getModule-- ?

Show
Herwig Hochleitner added a comment - Ghadi: does that mean, that we can't even get at the Class object to call http://download.java.net/java/jdk9/docs/api/java/lang/Class.html#getModule-- ?
Hide
Alex Miller added a comment -

My understanding (which may be faulty) is that the new module system is independent from the classloader setup (although the base classloader hierarchy has changed a bit) and that reflection is still largely the same with respect to examining classes and methods. But you may run into issues with invoking constructors or methods that are not allowed according to access restrictions. I'm not sure if the ability to load classes is affected (but I didn't think so).

Show
Alex Miller added a comment - My understanding (which may be faulty) is that the new module system is independent from the classloader setup (although the base classloader hierarchy has changed a bit) and that reflection is still largely the same with respect to examining classes and methods. But you may run into issues with invoking constructors or methods that are not allowed according to access restrictions. I'm not sure if the ability to load classes is affected (but I didn't think so).

People

Vote (1)
Watch (7)

Dates

  • Created:
    Updated: