Hi,
I reworked the checks and simplified: * It now creates a single methodhandle to check if a class is open for us (groovy) to call setAccessible * If this is not the case, we do not call setAccessible. Instead we filter all the AccessibleObect instances and only return those which the new Java 9 API AccessibleObject.canAccess returns true. This makes everything pass with Java 9. More funny: The Gradle 4.0 versions works and you can bootstrap the whole compilation!!! No 4.1 preview release needed (interestingly it now breaks with 4.1…)! No warnings, all bootstraps and compiles! Tests seem to pass, at least not more failed tests than before the patch with Java 9. Have fun in reviewing, the combines MethodHandle is just cool 😊 I only get the following warning, but I am not sure where it comes from (maybe it’s just Gradle): WARNING: Please consider reporting this to the maintainers of worker.org.gradle.internal.reflect.JavaMethod WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations WARNING: All illegal access operations will be denied in a future release Uwe ----- Uwe Schindler uschind...@apache.org ASF Member, Apache Lucene PMC / Committer Bremen, Germany http://lucene.apache.org/ From: Uwe Schindler [mailto:u...@thetaphi.de] Sent: Sunday, July 9, 2017 2:47 PM To: dev@groovy.apache.org Subject: RE: trySetAccessible for Java 9 Hi, I fixed the problem by removeing an obsolete check. I have no idea why it now works, but it runs without any warnings now! See my updated patch: https://github.com/apache/groovy/compare/master...uschindler:java9/moduleCheck I will later improve the patch by precompiling the methodhandle to one single call using bindto and other magic, so we don’t need 2 calls! Uwe ----- Uwe Schindler Achterdiek 19, D-28357 Bremen http://www.thetaphi.de <http://www.thetaphi.de/> eMail: u...@thetaphi.de <mailto:u...@thetaphi.de> From: Uwe Schindler [mailto:uschind...@apache.org] Sent: Sunday, July 9, 2017 2:34 PM To: dev@groovy.apache.org <mailto:dev@groovy.apache.org> Subject: RE: trySetAccessible for Java 9 Hi, Here is my proposal: https://github.com/apache/groovy/compare/master...uschindler:java9/moduleCheck This only calls setAccessible, if the module of the class is open to Groovy’s module and package. This no longer prints any warning, but now it has the following problem – which is horrible to me, why does Groovy try to do this? WARNING: An illegal reflective access operation has occurred WARNING: Illegal reflective access by org.codehaus.groovy.vmplugin.v7.Java7$1 (…) to constructor java.lang.invoke.MethodHandles$Lookup(java.lang.Class,int) WARNING: Please consider reporting this to the maintainers of org.codehaus.groovy.vmplugin.v7.Java7$1 WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations WARNING: All illegal access operations will be denied in a future release I also get a “stream closed” error while trying to “compile-groovy” ant task (I think that’s related to the above). Uwe ----- Uwe Schindler uschind...@apache.org <mailto:uschind...@apache.org> ASF Member, Apache Lucene PMC / Committer Bremen, Germany http://lucene.apache.org/ From: Uwe Schindler [mailto:uschind...@apache.org] Sent: Sunday, July 9, 2017 1:13 PM To: dev@groovy.apache.org <mailto:dev@groovy.apache.org> Subject: RE: trySetAccessible for Java 9 Hi, thanks for the insight. I was just not sure where this synthetic class name came from. I think we should ask on the Jigsaw mailing list if the warning was intended to be also printed by the trySetAccessible call. IMHO, trySetAccessible is a new API, so it should *not* grant access nor print a warning. The Jigsaw kill switch should have no effect on trySetAccessible! In contrast, setAccessible should behave as it does with the Jigsaw kill switch enabled. Nevertheless, I’d change the code in the following way: Instead of trySetAccessible method handle, use a methodhandle to get the Module instance of the Groovy runtime (fetch it once in the static initializer -- should be the unnamed module, but we should be flexible here) and when trying to make everything accessible in a class, use the same methodhandle to get the module, too. If the module if different, don’t try to make anything accessible at all and just return with empty array. In the future we may try improve the whole thing my looking into the module properties (“open” packages), but as a quick fix this is fine. I will modify my patch proposal to do this. Finally I have the question: Is it really needed to keep trying to make everything accessible by default in Groovy 3 ??? Maybe you should break backwards compatibility and no longer do this by default. This was IMHO a mis-design of the Groovy language from the beginning. This is also a security issue, as every groovy module can corrupt the whole JVM. This is also one reason why Elasticsearch moved to their own scripting language, as the cowboy “setAccessible” everywhere is a serious security issue! Uwe ----- Uwe Schindler uschind...@apache.org <mailto:uschind...@apache.org> ASF Member, Apache Lucene PMC / Committer Bremen, Germany http://lucene.apache.org/ From: John Wagenleitner [mailto:john.wagenleit...@gmail.com] Sent: Saturday, July 8, 2017 10:40 PM To: dev@groovy.apache.org <mailto:dev@groovy.apache.org> Subject: Re: trySetAccessible for Java 9 Looks like the InjectedInvoker [1] is an implementation detail of the MH lookup, probably used to allow calling the caller sensitive method. I did not think that trySetAccessible prevents the message from appearing, so even using that new method wont get rid of the warning even with the default of --illegal-access=permit. If a direct call or reflection is used instead of a MethodHandle it still prints the warning for the class where the direct call or reflective call is made. So semantically, as Cédric mentioned, that new method may make sense to use but it doesn't seem that it would eliminate the warning as far as I can see. [1] http://hg.openjdk.java.net/jigsaw/jake/jdk/file/f140e400a7f0/src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java#l1167 On Wed, Jul 5, 2017 at 2:02 PM, Uwe Schindler <uschind...@apache.org <mailto:uschind...@apache.org> > wrote: Hi, unfortunately the MethodHandle approach did not work without a small modification: java.lang.IllegalAccessException: Attempt to lookup caller-sensitive method using restricted lookup object As the trySetAccessible method is caller sensitive, you cannot get a MethodHandle from it using a public lookup. By changing the code to use a normal (private) lookup, it works (as expected). There are no security implications by that as we only access public methods. Only the lookup object needs the “owner” class to inject right caller sensitiveness. The private lookup (private to CachedClass) is allowed to get the method handle (and it should also be kept private inside CachedClass, otherwise you violate security!!!) I updated by branch: https://github.com/apache/groovy/compare/master...uschindler:java9/trySetAccessible This already helps when starting gradle, because as soon as the compileGroovy tasks are starting, you are using the bootstrapped JAR file. The CachedClass problem is fixed, no more illegal reflective acceses, but I got a new one from a bytecode generated class (!?): WARNING: An illegal reflective access operation has occurred WARNING: Illegal reflective access by org.codehaus.groovy.reflection.InjectedInvoker/1364880320 (file:/C:/Users/Uwe%20Schindler/Projects/groovy/target/classes/java/main/) to method java.lang.Object.finalize() WARNING: Please consider reporting this to the maintainers of org.codehaus.groovy.reflection.InjectedInvoker/1364880320 WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations WARNING: All illegal access operations will be denied in a future release As you see the whole thing got better, but now we have the same problem in org.codehaus.groovy.reflection.InjectedInvoker, but this one is synthetic. It looks like we must change the bytecode of that, too. But here we are lucky: We can use the detected Java 9 version and just create different bytecode at runtime depending on Java version? I have not looked at this, I just verified that my branch works with Java 9 build 175. I have not looked at VMPlugin stuff, that should be done by somebody else. But in that case: how about using a multi-release jar in that case? I know there is no support to create those in Maven/Gradle, but I am sure one can script it! IMHO: I would on Java 9 never ever use the array setAccessible method. You can be sure that it throws an exception in most cases, so why even try and take the cost of And I am not sure if setAccessible with array will not also print warnings, once Alan Bateman & Co. fixed this bug (it is a bug)! Uwe ----- Uwe Schindler uschind...@apache.org <mailto:uschind...@apache.org> ASF Member, Apache Lucene PMC / Committer Bremen, Germany http://lucene.apache.org/ From: Cédric Champeau [mailto:cchamp...@apache.org <mailto:cchamp...@apache.org> ] Sent: Wednesday, July 5, 2017 8:12 PM To: Uwe Schindler <uschind...@apache.org <mailto:uschind...@apache.org> > Cc: dev@groovy.apache.org <mailto:dev@groovy.apache.org> Subject: Re: trySetAccessible for Java 9 Thanks Uwe! To test with JDK 9 you'll need Gradle 4.1-milestone-1. I know Jochen has some special setup to make it work on previous releases of Gradle but I didn't try that. 2017-07-05 20:09 GMT+02:00 Uwe Schindler <uschind...@apache.org <mailto:uschind...@apache.org> >: Here is my quick patch: <https://github.com/apache/groovy/compare/master...uschindler:java9/trySetAccessible> https://github.com/apache/groovy/compare/master...uschindler:java9/trySetAccessible Sorry for my ignorance, but how to run tests with Java 9? Gradle fails for me to launch daemon! Uwe ----- Uwe Schindler uschind...@apache.org <mailto:uschind...@apache.org> ASF Member, Apache Lucene PMC / Committer Bremen, Germany http://lucene.apache.org/ From: Uwe Schindler [mailto:uschind...@apache.org <mailto:uschind...@apache.org> ] Sent: Wednesday, July 5, 2017 7:27 PM To: dev@groovy.apache.org <mailto:dev@groovy.apache.org> ; cchamp...@apache.org <mailto:cchamp...@apache.org> Subject: RE: trySetAccessible for Java 9 Working on it. I just looked at the code and found out that it already has a „fallback“ mechanism: It first tries setAccessible(array, true) and then falls back to do it one by one. I think with Java 9, wenn cannot do this. So I’d change that to: * Get methodhandle in static initializer, if not there set to NULL * In the makeAccessible method check for nullness of methodhandle: if null proceed as before, if not do a for-loop and call trySetAccesible() on all, ignoring return value. ----- Uwe Schindler uschind...@apache.org <mailto:uschind...@apache.org> ASF Member, Apache Lucene PMC / Committer Bremen, Germany http://lucene.apache.org/ From: Cédric Champeau [mailto:cchamp...@apache.org] Sent: Wednesday, July 5, 2017 7:10 PM To: dev@groovy.apache.org <mailto:dev@groovy.apache.org> Cc: Russel Winder <rus...@winder.org.uk <mailto:rus...@winder.org.uk> > Subject: Re: trySetAccessible for Java 9 Thanks Uwe, patches/PRs are very welcome :) I did miss your suggestion, sorry I wasn't able to follow everything on this list lately. The risk I saw was that the MethodHandle class wasn't always available, but for 2.4+, it's not a problem! 2017-07-05 19:07 GMT+02:00 Uwe Schindler <uschind...@apache.org <mailto:uschind...@apache.org> >: Hi, I made this suggestion about a month ago! In Lucene/Elasticsearch we do everything with MethodHandles that requires new Java 9 APIs (currently Elasticsearch’s Painless Script engine is the first one that uses indy string concats!). In general I would not use an if/then/else construct at all. Just try to get a MethodHandle to trySetAccessible(), if this fails get a MethodHandle to a local/private method with same signature. Finally you may need to adapt the MethodHandle to the right types and then call it _always_ with correct casting to make javac use correct types. Be sure to make the MethodHandle a static final constant somewhere! This removed the need for a if/then/else on every call. I may provide a patch, if you like. I’d just need some directions where to look at. Should be a 10 liner. Uwe ----- Uwe Schindler uschind...@apache.org <mailto:uschind...@apache.org> ASF Member, Apache Lucene PMC / Committer Bremen, Germany http://lucene.apache.org/ From: Cédric Champeau [mailto:cchamp...@apache.org <mailto:cchamp...@apache.org> ] Sent: Wednesday, July 5, 2017 6:55 PM To: Russel Winder <rus...@winder.org.uk <mailto:rus...@winder.org.uk> > Cc: dev@groovy.apache.org <mailto:dev@groovy.apache.org> Subject: Re: trySetAccessible for Java 9 Actually I'm realizing that the `MethodHandle` API came with Java 7. So we _can_ compile against it. So I guess an option is to have the method handle redirect to `trySetAccessible` if the detected runtime is Java 9, and a backport method if < 9. 2017-07-05 18:41 GMT+02:00 Russel Winder <rus...@winder.org.uk <mailto:rus...@winder.org.uk> >: On Wed, 2017-07-05 at 18:28 +0200, Cédric Champeau wrote: > […] > Any suggestion? How about leave Groovy 2.x as a "can only build on JDK8", and put all effort for a JDK9 build on Groovy 3.x which, as I understand it requires JDK8 as a runtime. This would seem to minimise hassle and maximise forward-looking benefit. Unless I am missing something. -- Russel. ============================================================================= Dr Russel Winder t:+44 20 7585 2200 <tel:%2B44%2020%207585%202200> voip:sip: russel.win...@ekiga.net <mailto:russel.win...@ekiga.net> 41 Buckmaster Road m:+44 7770 465 077 <tel:%2B44%207770%20465%20077> xmpp:rus...@winder.org.uk <mailto:xmpp%3arus...@winder.org.uk> London SW11 1EN, UK w: www.russel.org.uk <http://www.russel.org.uk> skype:russel_winder
