Bug 58271 - script task implementation should use reflection API to find if javax.script.ScriptEngineManager is available
Summary: script task implementation should use reflection API to find if javax.script....
Status: RESOLVED FIXED
Alias: None
Product: Ant
Classification: Unclassified
Component: Optional Tasks (show other bugs)
Version: 1.9.5
Hardware: All All
: P2 normal (vote)
Target Milestone: 1.9.7
Assignee: Ant Notifications List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-22 09:57 UTC by a.sundararajan
Modified: 2017-02-18 05:03 UTC (History)
1 user (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description a.sundararajan 2015-08-22 09:57:56 UTC
Based on reading 1.9.5 sources:

http://grepcode.com/file/repo1.maven.org/maven2/org.apache.ant/ant/1.9.5/org/apache/tools/ant/util/ScriptRunnerCreator.java#ScriptRunnerCreator.createRunner%28java.lang.String%2Cjava.lang.String%2Cjava.lang.String%29

http://grepcode.com/file/repo1.maven.org/maven2/org.apache.ant/ant/1.9.5/org/apache/tools/ant/util/LoaderUtils.java#LoaderUtils.classNameToResource%28java.lang.String%29

I find that <script> task implementation uses ClassLoader.getResource is used to locate a .class resource URL. Based on whether that returns null or not, ScriptRunnerBase is created. But per documentation of ClassLoader.getResource (http://docs.oracle.com/javase/8/docs/api/java/lang/ClassLoader.html#getResource-java.lang.String-), getResource is meant for only audio/text etc. -  / A resource is some data (images, audio, text, etc) that can be accessed by class code in a way that is independent of the location of the code./ Never talks about ".class" 'file' as a resource. Besides, reflection API may be used to check if javax.script.ScriptEngineManager class is available or not in the JDK. ClassLoader.getResource on .class resource can be avoided.
Comment 1 Stefan Bodewig 2015-08-22 17:02:39 UTC
Are you experiencing any problem with the current approach?
Comment 2 a.sundararajan 2015-08-25 10:48:19 UTC
(In reply to Stefan Bodewig from comment #1)
> Are you experiencing any problem with the current approach?

Hi, I am working on jdk9. The behaviour of ClassLoader.getResource is expected to change in the modular ("jigsaw") jdk9 (http://openjdk.java.net/projects/jigsaw/). Retrieving .class resource URL from a named module will not work as expected.

Given that there is cleaner alternative - namely reflection API, it is better to use java reflection to check javax.script.ScriptEngineManager exists or not in the JDK configured. Is there any particular reason why this ClassLoader.getResource is preferred? If so, please let me know.
Comment 3 Stefan Bodewig 2015-08-25 19:15:03 UTC
TBH I don't remember why the code was written the way it is and haven't been involved with writing it at all - just wanted to avoid fixing code for cosmetic reasons that wasn't broken.

Looking at the code my guess is the special case for BSF that immediately before the Class.forName call is required for it to work when using BSF and the code needs a way to ensure BSF is actually there before modifying the classloader hierarchy.

The problem we should really talk about is LoaderUtils.classNameToResource which is also used inside the JUnit task.  We use it to tell people we've found multiple versions of JUnit.  Not only that, we also tell them where we've found them.  How would we achieve the same with using reflection alone?

The LoaderUtils.classExists that uses LoaderUtils.classNameToResource is only used inside the BSF classloader hack so shouldn't be a problem if we manage to fix the hack.
Comment 4 a.sundararajan 2015-08-26 10:44:44 UTC
(In reply to Stefan Bodewig from comment #3)
> TBH I don't remember why the code was written the way it is and haven't been
> involved with writing it at all - just wanted to avoid fixing code for
> cosmetic reasons that wasn't broken.
> 
> Looking at the code my guess is the special case for BSF that immediately
> before the Class.forName call is required for it to work when using BSF and
> the code needs a way to ensure BSF is actually there before modifying the
> classloader hierarchy.
> 
> The problem we should really talk about is LoaderUtils.classNameToResource
> which is also used inside the JUnit task.  We use it to tell people we've
> found multiple versions of JUnit.  Not only that, we also tell them where
> we've found them.  How would we achieve the same with using reflection alone?
> 
> The LoaderUtils.classExists that uses LoaderUtils.classNameToResource is
> only used inside the BSF classloader hack so shouldn't be a problem if we
> manage to fix the hack.

ClassLoader.getResource behaviour change is *only* about classes in named modules (like javax.script.ScriptEngineManager and other JDK platform classes which will be in named modules). The use ClassLoader.getResource for other classes like junit or anything that is deployed via classpath or located via some custom class loader - is not be a problem. That should work fine as it does in the current JDK versions. So, if the use of ClassLoader.getResource for platform classes can be avoided, ant tasks would be fine.
Comment 5 Stefan Bodewig 2015-08-28 16:43:24 UTC
IIUC we could modify ScriptRunnerCreator so that it still uses ClassLoader.getResource if the selected engine is BSF but uses Class.forName for the javax.script engine, right?

This should address the changes Java9 and allow us to fiddle with classloaders before loading BSF at the same time.
Comment 6 a.sundararajan 2015-08-31 02:59:33 UTC
(In reply to Stefan Bodewig from comment #5)
> IIUC we could modify ScriptRunnerCreator so that it still uses
> ClassLoader.getResource if the selected engine is BSF but uses Class.forName
> for the javax.script engine, right?
> 
> This should address the changes Java9 and allow us to fiddle with
> classloaders before loading BSF at the same time.

Yes. That should work.
Comment 7 Stefan Bodewig 2015-11-07 17:44:27 UTC
should be fixed with git commit 4ccca1f
Comment 8 Björn Kautler 2017-02-17 18:04:43 UTC
Actually this was not a bug in Ant but in Java.

At least I would interpret it in that way that this error happens with Java 9 build 114, but works fine with the latest released build 156 which is the case.

So if you liked the old code better you now have the choice to change it back. Excpet of course if the Java bug is, that it works again in the latest build. :-)
Comment 9 Stefan Bodewig 2017-02-18 05:03:39 UTC
Thanks Björn

back then it seemed as if the change would be required for Java9 as the way Java wrked was about to change. Now it looks the Java0 team has changed their minds again. I'm not convinced build 156 is the final call :-)

Apart from that it probably is good the old code works again as it is still used for JUnit. Aftre re-reading this thread it would have failed for JUnit as well under 9b114 if junit-jar was a named module. Well, in theory the junit devs could decide to ship junit as a Java0 module in the future.

For Ant 1.10.x I'd be willing to rewrite the whole access of javax.script to not use reflection at all as we are guaranteed to find it - Ant 1.10 requires Java8 at runtime.