Discussion:
Groovy with Jigsaw
(too old to reply)
Jochen Theodorou
2015-09-10 22:40:15 UTC
Permalink
Hi,

we are currently investigating problems Groovy has with Jigsaw. A lot
changed with jigsaw, and since Groovy does often not use standard
methods, a lot fails.

But first I would like to ask something that I haven't seen covered in
http://openjdk.java.net/projects/jigsaw/quick-start or
http://openjdk.java.net/projects/jigsaw/spec/sotms/ Maybe I did not see
it, since it is late here. I hope the questions should not better have
gone to the expert group comment list. It was not fully clear to me
where goes what. Anyway...

As far as I understood a classloader can have a N modules plus the
unnamed module. I assume a the children of a classloader do not
automatically share modules (or not at all?) with their children.
Do assume right in that there is no way to create a module at runtime?

Next question is about runtime generated classes. And I mean here
classes generated by user code at runtime, as well as proxies, code for
reflection and invokedynamic and all the other bytecode generating
facilities. Will they all go into the unnamed module?

Assuming they go there (how else can you choose from those N modules),
what access rights to the modules will they have? I assume to everything
that is exported, and for that it does not matter if it is the same
class loader or a parent or any other loader.

I further assume you can load a module at runtime... by for example
using URLClassLoader. Now let us say there is a classloader for module
A, which exports some packages to module B. Must module B then be loaded
in the same classloader (or a parent) as A? If a child loader can load B
and allow access according to the exports rules. What happens if
multiple children each have their own B, and maybe those B are totally
unrelated to each other? I assume this is possible.

Now a more practical one... In Groovy we have code like this:

try {
AccessController.doPrivileged(new PrivilegedAction() {
public Object run() {
c.setAccessible(true);
return null;
}
});
}
catch (SecurityException e) {
// IGNORE
}

which is code supposed to "check" if we can access the class member or
if any security manager will disallow this. In this case the method
becomes unavailable for invocation... Now jigsaw changes do make the
setAccessible method also throws
java.lang.reflect.InaccessibleObjectException. What is the suggested way
to handle this for code that has to be also compatible to pre JDK9 code?
catch RuntimeException, check the class name and rethrow if the name
does not fit?

And then a question probably a bit more complicated...

Groovy uses reflection/runtime generated classes/invokedynamic to invoke
methods. That means the method invocation point (for reflection, the
class that calls the invoke method) is most often in the Groovy runtime.
And even if Groovy would define its own module, those runtime generated
classes would not be in the same module (or is there a way for this?)
Now assume somebody wrote this class in Groovy:

/* code without sense following */
class MyFactory {
public Factory getFactory() {return new FactoryImpl()}
}
interface Factory{String foo()}
class FactoryImpl implements Factory{
String foo() { return internal()+"foo" }
String internal() {"Factory"}
}

and let us assume MyFactory and Factory are in a package, which is
exported, FactoryImpl not. Now... since Groovy is currently in no module
and since it needs to use reflection to execute the internal() method
call here, does this mean the call will fail, because the object is
inaccessible? Actually I assume we would not even see the method.

If that assumption is right, does this means every application that uses
modules and is at least partially written in Groovy, has to export each
and every package to Groovy, in order to allow it to do method calls?
That is assuming Groovy has a module by then. Without it I assume it
cannot be made.

Well... enough for now, further questions will surely develop later on.

bye Jochen Theodorou
--
Jochen "blackdrag" Theodorou
blog: http://blackdragsview.blogspot.com/
Uwe Schindler
2015-09-10 22:57:31 UTC
Permalink
Hi,
Post by Jochen Theodorou
try {
AccessController.doPrivileged(new PrivilegedAction() {
public Object run() {
c.setAccessible(true);
return null;
}
});
}
catch (SecurityException e) {
// IGNORE
}
which is code supposed to "check" if we can access the class member or if
any security manager will disallow this. In this case the method becomes
unavailable for invocation... Now jigsaw changes do make the setAccessible
method also throws java.lang.reflect.InaccessibleObjectException. What is
the suggested way to handle this for code that has to be also compatible to
pre JDK9 code?
catch RuntimeException, check the class name and rethrow if the name does
not fit?
I already opened an issue about this: https://issues.apache.org/jira/browse/GROOVY-7587

This makes Groovy unuseable at the moment with JIGSAW. Elasticsearch is affected by this, but also the Ant-based build scripts used to build Lucene and Forbiddenapis (both use Groovy from Ant). I think the only "quick" fix would be to check RuntimeException, check class type, otherwise rethrow.

I also wanted to ask the question: setAccessible() already throws some checked exceptions, why invent a new one that’s no longer checked? Could it not simply be one already there - one of the checked ones - or a new one just subclassing ReflectiveOperationException? This is completely inconsistent to other core reflection APIs. Why have a totally unexpected new one?

Uwe

-----
Uwe Schindler
***@apache.org
ASF Member, Apache Lucene PMC / Committer
Bremen, Germany
http://lucene.apache.org/
Robert Muir
2015-09-10 23:25:30 UTC
Permalink
I agree with Uwe. Practically this just makes more code to fix. I dont see
any advantage of using a new exception, what is the rationale? Using an
existing exception means existing try catch blocks will take care in many
of these cases and just slow the adoption of java 9 even more.
Post by Uwe Schindler
Hi,
Post by Jochen Theodorou
try {
AccessController.doPrivileged(new PrivilegedAction() {
public Object run() {
c.setAccessible(true);
return null;
}
});
}
catch (SecurityException e) {
// IGNORE
}
which is code supposed to "check" if we can access the class member or if
any security manager will disallow this. In this case the method becomes
unavailable for invocation... Now jigsaw changes do make the
setAccessible
Post by Uwe Schindler
Post by Jochen Theodorou
method also throws java.lang.reflect.InaccessibleObjectException. What is
the suggested way to handle this for code that has to be also compatible to
pre JDK9 code?
catch RuntimeException, check the class name and rethrow if the name does
not fit?
https://issues.apache.org/jira/browse/GROOVY-7587
Post by Uwe Schindler
This makes Groovy unuseable at the moment with JIGSAW. Elasticsearch is
affected by this, but also the Ant-based build scripts used to build Lucene
and Forbiddenapis (both use Groovy from Ant). I think the only "quick" fix
would be to check RuntimeException, check class type, otherwise rethrow.
Post by Uwe Schindler
I also wanted to ask the question: setAccessible() already throws some
checked exceptions, why invent a new one that’s no longer checked? Could it
not simply be one already there - one of the checked ones - or a new one
just subclassing ReflectiveOperationException? This is completely
inconsistent to other core reflection APIs. Why have a totally unexpected
new one?
Post by Uwe Schindler
Uwe
-----
Uwe Schindler
ASF Member, Apache Lucene PMC / Committer
Bremen, Germany
http://lucene.apache.org/
Alan Bateman
2015-09-11 07:39:09 UTC
Permalink
Post by Uwe Schindler
I also wanted to ask the question: setAccessible() already throws some checked exceptions, why invent a new one that’s no longer checked? Could it not simply be one already there - one of the checked ones - or a new one just subclassing ReflectiveOperationException? This is completely inconsistent to other core reflection APIs. Why have a totally unexpected new one?
Right, there is compatibility issue here (third item in the "Risks and
Assumptions" section of JEP 261 [1]).

The setAccessible methods don't declare any checked exceptions. The only
exception they declare is SecurityException. This exception make sense
when running with a security manager and the specified permission check
fails. This exception is the wrong exception to fail with when the
member is not an an exported package as this is nothing to do with the
security manager. Sadly, these methods do throw SecurityException when
attempted on a constructor of java.lang.Class, I've been trying to dig
into the history on this oddity.

Anyway, retrofitting these methods to throw ReflectiveOperationException
(or sub-type) isn't really an option because this is a checked exception
so it would add a source compatibility issue to the list. For now, the
prototype API changes these methods to throw InaccessibleObjectException
and we'll have to see the impact of this. I hope we aren't force to
change this to throw SecurityException as it's the wrong exception, but
less impact compatibility-wise because there is some chance that
existing code might handle that already.

-Alan.

[1] http://openjdk.java.net/jeps/261
Cédric Champeau
2015-09-11 07:47:05 UTC
Permalink
For what it's worth, the issue that triggered this conversation is the one
reported by Uwe. For Groovy we have a chicken and egg problem for testing,
because this change breaks Groovy, and Groovy uses Gradle to build. Since
Gradle itself uses Groovy, we have no compatible build tool to test the
fix... So it's very problematic. Also the build that we set up failed with:

[23:53:08]W: [Gradle failure report] Caused by: java.lang.VerifyError:
class com.google.common.reflect.Element overrides final method
java.lang.reflect.AccessibleObject.setAccessible(Z)V
[23:53:08]W: [Gradle failure report] at
java.lang.ClassLoader.defineClass1(***@9.0/Native Method)
[23:53:08]W: [Gradle failure report] at
java.lang.ClassLoader.defineClass(***@9.0/ClassLoader.java:820)
[23:53:08]W: [Gradle failure report] at
java.security.SecureClassLoader.defineClass(***@9.0
/SecureClassLoader.java:152)

Which indicates the change to setAccessible also broke Guava, which is used
by Gradle. So it's going to be very complicated to even try to fix the
issue in those conditions. Anyway, it doesn't seem a good idea to introduce
a new exception type. Even if it is semantically a bit problematic,
wouldn't make ReflectiveOperationException a subclass of SecurityException
an option?
Post by Alan Bateman
Post by Uwe Schindler
I also wanted to ask the question: setAccessible() already throws some
checked exceptions, why invent a new one that’s no longer checked? Could it
not simply be one already there - one of the checked ones - or a new one
just subclassing ReflectiveOperationException? This is completely
inconsistent to other core reflection APIs. Why have a totally unexpected
new one?
Right, there is compatibility issue here (third item in the "Risks and
Assumptions" section of JEP 261 [1]).
The setAccessible methods don't declare any checked exceptions. The only
exception they declare is SecurityException. This exception make sense when
running with a security manager and the specified permission check fails.
This exception is the wrong exception to fail with when the member is not
an an exported package as this is nothing to do with the security manager.
Sadly, these methods do throw SecurityException when attempted on a
constructor of java.lang.Class, I've been trying to dig into the history on
this oddity.
Anyway, retrofitting these methods to throw ReflectiveOperationException
(or sub-type) isn't really an option because this is a checked exception so
it would add a source compatibility issue to the list. For now, the
prototype API changes these methods to throw InaccessibleObjectException
and we'll have to see the impact of this. I hope we aren't force to change
this to throw SecurityException as it's the wrong exception, but less
impact compatibility-wise because there is some chance that existing code
might handle that already.
-Alan.
[1] http://openjdk.java.net/jeps/261
Alan Bateman
2015-09-11 08:16:04 UTC
Permalink
Post by Cédric Champeau
For what it's worth, the issue that triggered this conversation is the
one reported by Uwe. For Groovy we have a chicken and egg problem for
testing, because this change breaks Groovy, and Groovy uses Gradle to
build. Since Gradle itself uses Groovy, we have no compatible build
tool to test the fix... So it's very problematic. Also the build that
class com.google.common.reflect.Element overrides final method
java.lang.reflect.AccessibleObject.setAccessible(Z)V
[23:53:08]W:[Gradle failure report] at
[23:53:08]W:[Gradle failure report] at
[23:53:08]W:[Gradle failure report] at
Which indicates the change to setAccessible also broke Guava, which is
used by Gradle. So it's going to be very complicated to even try to
fix the issue in those conditions. Anyway, it doesn't seem a good idea
to introduce a new exception type. Even if it is semantically a bit
problematic, wouldn't make ReflectiveOperationException a subclass of
SecurityException an option?
The right exception for this case might need more consideration but I
assume the underlying issue must be a failed attempt to get access to a
member of a type in a non-exported package.

Is there any way to run this with -Dsun.reflect.debugModuleAccessChecks
to see if you get any stack traces to debug this?

-Alan
Jochen Theodorou
2015-09-11 09:21:43 UTC
Permalink
Post by Alan Bateman
Post by Cédric Champeau
For what it's worth, the issue that triggered this conversation is the
one reported by Uwe. For Groovy we have a chicken and egg problem for
testing, because this change breaks Groovy, and Groovy uses Gradle to
build. Since Gradle itself uses Groovy, we have no compatible build
tool to test the fix... So it's very problematic. Also the build that
class com.google.common.reflect.Element overrides final method
java.lang.reflect.AccessibleObject.setAccessible(Z)V
[23:53:08]W:[Gradle failure report] at
[23:53:08]W:[Gradle failure report] at
[23:53:08]W:[Gradle failure report] at
Which indicates the change to setAccessible also broke Guava, which is
used by Gradle. So it's going to be very complicated to even try to
fix the issue in those conditions. Anyway, it doesn't seem a good idea
to introduce a new exception type. Even if it is semantically a bit
problematic, wouldn't make ReflectiveOperationException a subclass of
SecurityException an option?
The right exception for this case might need more consideration but I
assume the underlying issue must be a failed attempt to get access to a
member of a type in a non-exported package.
Is there any way to run this with -Dsun.reflect.debugModuleAccessChecks
to see if you get any stack traces to debug this?
the error in Guava is not a failed attempt, it is a VerifyError, because
AccessibleObject#setAccessible is now final and
com.google.common.reflect.Element overrides the method.

As far as I have seen Guava makes a parallel reflective structure of a
class to be able to better handle them. A the same time they proxy to
their counter parts, but do also implement interfaces like Member. That
requires for example to override setAccessible, which will just make the
same call on the delegate. I don't see how they can easily fix that
without giving up their mirror structure and have it exchangeable with
the java reflective classes at the same time. But I am not on the guava
team, so they know hopefully better

bye blackdrag
--
Jochen "blackdrag" Theodorou
blog: http://blackdragsview.blogspot.com/
Alan Bateman
2015-09-11 09:33:21 UTC
Permalink
Post by Jochen Theodorou
the error in Guava is not a failed attempt, it is a VerifyError,
because AccessibleObject#setAccessible is now final and
com.google.common.reflect.Element overrides the method.
As far as I have seen Guava makes a parallel reflective structure of a
class to be able to better handle them. A the same time they proxy to
their counter parts, but do also implement interfaces like Member.
That requires for example to override setAccessible, which will just
make the same call on the delegate. I don't see how they can easily
fix that without giving up their mirror structure and have it
exchangeable with the java reflective classes at the same time. But I
am not on the guava team, so they know hopefully better
Okay, I mis-read one of the mails and assumed that setAccessible was
failing and leading to other errors.

In the current builds then setAccessible is final, and yes, this is an
incompatible change. The reason for this is that the method is now @CS.
I think we need to consider changing this so that setAccessible is
overridden in the final Field, Constructor, and Method classes instead.

-Alan
Stephen Colebourne
2015-09-11 09:37:10 UTC
Permalink
I tried compiling OpenGamma via maven and ran into this straight away:

java.lang.VerifyError: class com.google.common.reflect.Element
overrides final method
java.lang.reflect.AccessibleObject.setAccessible(Z)V
at java.lang.ClassLoader.defineClass1(***@9.0/Native Method)
at java.lang.ClassLoader.defineClass(***@9.0/ClassLoader.java:820)
at java.security.SecureClassLoader.defineClass(***@9.0/SecureClassLoader.java:152)

The Guava code is:
@Override public final void setAccessible(boolean flag) throws
SecurityException {
accessibleObject.setAccessible(flag);
}

So yes, looking at alternative ways to make that method final is
probably an essential.

Stephen
Post by Jochen Theodorou
the error in Guava is not a failed attempt, it is a VerifyError, because
AccessibleObject#setAccessible is now final and
com.google.common.reflect.Element overrides the method.
As far as I have seen Guava makes a parallel reflective structure of a
class to be able to better handle them. A the same time they proxy to their
counter parts, but do also implement interfaces like Member. That requires
for example to override setAccessible, which will just make the same call on
the delegate. I don't see how they can easily fix that without giving up
their mirror structure and have it exchangeable with the java reflective
classes at the same time. But I am not on the guava team, so they know
hopefully better
Okay, I mis-read one of the mails and assumed that setAccessible was failing
and leading to other errors.
In the current builds then setAccessible is final, and yes, this is an
think we need to consider changing this so that setAccessible is overridden
in the final Field, Constructor, and Method classes instead.
-Alan
Jochen Theodorou
2015-09-11 09:46:25 UTC
Permalink
Post by Alan Bateman
Post by Jochen Theodorou
the error in Guava is not a failed attempt, it is a VerifyError,
because AccessibleObject#setAccessible is now final and
com.google.common.reflect.Element overrides the method.
As far as I have seen Guava makes a parallel reflective structure of a
class to be able to better handle them. A the same time they proxy to
their counter parts, but do also implement interfaces like Member.
That requires for example to override setAccessible, which will just
make the same call on the delegate. I don't see how they can easily
fix that without giving up their mirror structure and have it
exchangeable with the java reflective classes at the same time. But I
am not on the guava team, so they know hopefully better
Okay, I mis-read one of the mails and assumed that setAccessible was
failing and leading to other errors.
It does fail for any Groovy script, that invokes any method on an
Object, that has a declaring class, which is made inaccessible through
the module system, due to the new exception being thrown. Example:

ClassLoader.getSystemClassLoader().toString()
Post by Alan Bateman
In the current builds then setAccessible is final, and yes, this is an
I think we need to consider changing this so that setAccessible is
overridden in the final Field, Constructor, and Method classes instead.
Does @CS then mean guava will then possibly interfere with the security
manager logic, since it introduces a new layer of calling code in between?

bye Jochen
--
Jochen "blackdrag" Theodorou
blog: http://blackdragsview.blogspot.com/
Uwe Schindler
2015-09-11 11:00:02 UTC
Permalink
Just to conclude, there are 2 problems around setAccessible:

First, it throws a new Exception type. This makes almost any Groovy script fail at some point, because it tries to add some meta class on the scripting level, and this call setAccessible on all fields and methods of any class used in the script, only catching SecurityException but not the new one. My preferred solution would be to make the new InaccessibleObjectException as subclass of SecurityException (and add some documentation why this is done like that). Please note this does not differ from current Java 8 spec, where setAccessible may throw SecurityException without a security manager on some special objects (Class constructor). I agree, this is also strange (UOE would be more correct), but the same solution could be taken here, too.

Second: AccessibleObject#setAccessible was made final, this breaks "delegators" like the one in Google Guava, which is a library used in like 50% of all larger software projects. Changing methods to be final in abstract classes is always a hard break, because abstract classes are there to be extended. The workaround may be to make the implementations final (which they are already, because Method, Field,... classes are final). Maybe just move the implementation into the final subclasses (with stupid code duplication, alternatively use a static helper class to avoid the code duplication). We had a similar issue in Java 8 EA builds (reported by Lucene at that time), where isAnnotationPresent() was moved as *default* method to the interface AnnotatedObject and then the impl in Field/Method/... was removed. This caused compiler failures for code compiled with source/target 1.7. The fix was to define the method in all subclasses but delegate to the interface with some super-like construct.

Uwe

-----
Uwe Schindler
***@apache.org
ASF Member, Apache Lucene PMC / Committer
Bremen, Germany
http://lucene.apache.org/
-----Original Message-----
Sent: Friday, September 11, 2015 11:46 AM
To: Alan Bateman; Cédric Champeau
Subject: Re: Groovy with Jigsaw
Post by Alan Bateman
Post by Jochen Theodorou
the error in Guava is not a failed attempt, it is a VerifyError,
because AccessibleObject#setAccessible is now final and
com.google.common.reflect.Element overrides the method.
As far as I have seen Guava makes a parallel reflective structure of
a class to be able to better handle them. A the same time they proxy
to their counter parts, but do also implement interfaces like Member.
That requires for example to override setAccessible, which will just
make the same call on the delegate. I don't see how they can easily
fix that without giving up their mirror structure and have it
exchangeable with the java reflective classes at the same time. But I
am not on the guava team, so they know hopefully better
Okay, I mis-read one of the mails and assumed that setAccessible was
failing and leading to other errors.
It does fail for any Groovy script, that invokes any method on an Object, that
has a declaring class, which is made inaccessible through the module system,
ClassLoader.getSystemClassLoader().toString()
Post by Alan Bateman
In the current builds then setAccessible is final, and yes, this is an
I think we need to consider changing this so that setAccessible is
overridden in the final Field, Constructor, and Method classes instead.
manager logic, since it introduces a new layer of calling code in between?
bye Jochen
--
Jochen "blackdrag" Theodorou
blog: http://blackdragsview.blogspot.com/
Jochen Theodorou
2015-09-11 12:11:39 UTC
Permalink
Post by Uwe Schindler
First, it throws a new Exception type. This makes almost any Groovy script fail at some point, because it tries to add some meta class on the scripting level, and this call setAccessible on all fields and methods of any class used in the script, only catching SecurityException but not the new one. My preferred solution would be to make the new InaccessibleObjectException as subclass of SecurityException (and add some documentation why this is done like that). Please note this does not differ from current Java 8 spec, where setAccessible may throw SecurityException without a security manager on some special objects (Class constructor). I agree, this is also strange (UOE would be more correct), but the same solution could be taken here, too.
Second: AccessibleObject#setAccessible was made final, this breaks "delegators" like the one in Google Guava, which is a library used in like 50% of all larger software projects. Changing methods to be final in abstract classes is always a hard break, because abstract classes are there to be extended. The workaround may be to make the implementations final (which they are already, because Method, Field,... classes are final). Maybe just move the implementation into the final subclasses (with stupid code duplication, alternatively use a static helper class to avoid the code duplication). We had a similar issue in Java 8 EA builds (reported by Lucene at that time), where isAnnotationPresent() was moved as *default* method to the interface AnnotatedObject and then the impl in Field/Method/... was removed. This caused compiler failures for code compiled with source/target 1.7. The fix was to define the method in all subclasses but delegate to the interface with some super-like construc
t
.

It doesn't end there. I have yet to investigate what it means if the
system class loader is no URLClassLoader per default anymore. This will
have impact on Groovy's @Grab. This will cause problems with for example
loading jdbc drivers at runtime.

And if the assumptions about the module system in my initial post all
are correct (nobody commented on those), I see trouble for Groovy
calling methods in general. If we have to make a Groovy module and
modules written in Groovy have to allow Groovy access to internal APIs,
to be able to make normal method calls, then it means the module system
is bypassed, because then any other Groovy program from any other module
can use Groovy to access internals. Which also means Java programs can
use Groovy to exploit everything of any Groovy module.

And somebody has to explain to me why this is supposed to be different
for Nashorn. I would imagine it is even worse there, since Nashorn can
see internal APIs, that are hidden to others and since it is part of the
JDK.

bye blackdrag
--
Jochen "blackdrag" Theodorou
blog: http://blackdragsview.blogspot.com/
Alan Bateman
2015-09-11 12:47:38 UTC
Permalink
Post by Jochen Theodorou
Post by Uwe Schindler
First, it throws a new Exception type. This makes almost any Groovy
script fail at some point, because it tries to add some meta class on
the scripting level, and this call setAccessible on all fields and
methods of any class used in the script, only catching
SecurityException but not the new one. My preferred solution would be
to make the new InaccessibleObjectException as subclass of
SecurityException (and add some documentation why this is done like
that). Please note this does not differ from current Java 8 spec,
where setAccessible may throw SecurityException without a security
manager on some special objects (Class constructor). I agree, this is
also strange (UOE would be more correct), but the same solution could
be taken here, too.
Yes, this a incompatible change as InaccessibleObjectException is new.
We might need to look at this again but changing it to security
exception (or a sub-type) is really icky. One thing that would be useful
to find setAccessible usages in other libraries to see if security
exception is caught or handled.
Post by Jochen Theodorou
Post by Uwe Schindler
Second: AccessibleObject#setAccessible was made final
I have a patch to resolve this one, just needs more testing, there was
no intention to break anyone with this. My guess is that it is is very
rare to extent AccessibleObject outside of java.lang.reflect.
Post by Jochen Theodorou
It doesn't end there. I have yet to investigate what it means if the
system class loader is no URLClassLoader per default anymore. This
example loading jdbc drivers at runtime.
It's always been possible to configure the system class loader to be
something else. So I'm curious what you do if it is a URLClassLoader,
are you just looking for the class path?
Post by Jochen Theodorou
And somebody has to explain to me why this is supposed to be different
for Nashorn. I would imagine it is even worse there, since Nashorn can
see internal APIs, that are hidden to others and since it is part of
the JDK.
Nashorn is updated in the EA builds to work with modules, it is spinning
dynamic modules at run-time. I think it requires building up examples of
real needs before thinking about an API here.

-Alan
Jochen Theodorou
2015-09-11 15:46:59 UTC
Permalink
Am 11.09.2015 14:47, schrieb Alan Bateman:
[...]
Post by Alan Bateman
It's always been possible to configure the system class loader to be
something else. So I'm curious what you do if it is a URLClassLoader,
are you just looking for the class path?
Then it would not work. But since the cases for @Grab have not been
cases in which you normally do that, there was almost always the chance
to do this in practice. Now not anymore. Normally you don't need to do
this kind of thing either. There is for example GroovyRootLoader, which
we use to load the Groovy runtime, which is an URLClassLoader and can be
used... only in some cases like some jdbc drivers you need a higher
loader and that's where the system class loader was used in the past.
What is the alternative?
Post by Alan Bateman
Post by Jochen Theodorou
And somebody has to explain to me why this is supposed to be different
for Nashorn. I would imagine it is even worse there, since Nashorn can
see internal APIs, that are hidden to others and since it is part of
the JDK.
Nashorn is updated in the EA builds to work with modules, it is spinning
dynamic modules at run-time. I think it requires building up examples of
real needs before thinking about an API here.
My thought experiment is the following...

Have module A, with internal API in a.internal and public stuff in
a.public. Furthermore A is written in Groovy. Let us say there is a
public static method a.internal.BadClass#badMethod(), then any Groovy
program can do the following: a.internal.BadClass.badMethod(). And that
is because the method call will be done from Groovy. For A to be working
there has to be a Groovy module which is allowed to access the internal
API, since otherwise the classes there won't work.

But maybe this question should go to the expert group instead?

bye Jochen
--
Jochen "blackdrag" Theodorou
blog: http://blackdragsview.blogspot.com/
Remi Forax
2015-09-11 12:50:00 UTC
Permalink
Jochen,
@CS == @CallerSensitive which means that setAccessible may throw an error depending where you call (from which class).

Given that in Guava, setAccessible is called inside the Guava code,
then this guava code may now fail and throw an exception.

Rémi


----- Mail original -----
Envoyé: Vendredi 11 Septembre 2015 11:46:25
Objet: Re: Groovy with Jigsaw
Post by Alan Bateman
Post by Jochen Theodorou
the error in Guava is not a failed attempt, it is a VerifyError,
because AccessibleObject#setAccessible is now final and
com.google.common.reflect.Element overrides the method.
As far as I have seen Guava makes a parallel reflective structure of a
class to be able to better handle them. A the same time they proxy to
their counter parts, but do also implement interfaces like Member.
That requires for example to override setAccessible, which will just
make the same call on the delegate. I don't see how they can easily
fix that without giving up their mirror structure and have it
exchangeable with the java reflective classes at the same time. But I
am not on the guava team, so they know hopefully better
Okay, I mis-read one of the mails and assumed that setAccessible was
failing and leading to other errors.
It does fail for any Groovy script, that invokes any method on an
Object, that has a declaring class, which is made inaccessible through
ClassLoader.getSystemClassLoader().toString()
Post by Alan Bateman
In the current builds then setAccessible is final, and yes, this is an
I think we need to consider changing this so that setAccessible is
overridden in the final Field, Constructor, and Method classes instead.
manager logic, since it introduces a new layer of calling code in between?
bye Jochen
--
Jochen "blackdrag" Theodorou
blog: http://blackdragsview.blogspot.com/
Uwe Schindler
2015-09-11 09:07:30 UTC
Permalink
Hi,
Caused by: java.lang.VerifyError: class com.google.common.reflect.Element overrides final method java.lang.reflect.AccessibleObject.setAccessible(Z)V
This is caused by the fact that the new JDK makes setAccessible final in the base class AccessibleObject. The Google Guava class is a wrapper class around any AccessibleObject and delegates all calls to the object behind, while adding some convenience methods.



This is a clear backwards compatibility break. Maybe remove the final here! Otherwise Guava has to fix the code and completely remove the reflect.Element wrapper class (to me it looks useless, it just spares you a few modifier stuff because you can call isPublic()).



Uwe



-----

Uwe Schindler

***@apache.org

ASF Member, Apache Lucene PMC / Committer

Bremen, Germany

http://lucene.apache.org/



From: Cédric Champeau [mailto:***@gmail.com]
Sent: Friday, September 11, 2015 9:47 AM
To: Alan Bateman
Cc: Uwe Schindler; Jochen Theodorou; jigsaw-***@openjdk.java.net
Subject: Re: Groovy with Jigsaw



For what it's worth, the issue that triggered this conversation is the one reported by Uwe. For Groovy we have a chicken and egg problem for testing, because this change breaks Groovy, and Groovy uses Gradle to build. Since Gradle itself uses Groovy, we have no compatible build tool to test the fix... So it's very problematic. Also the build that we set up failed with:



[23:53:08]W: [Gradle failure report] Caused by: java.lang.VerifyError: class com.google.common.reflect.Element overrides final method java.lang.reflect.AccessibleObject.setAccessible(Z)V

[23:53:08]W: [Gradle failure report] at java.lang.ClassLoader.defineClass1(***@9.0/Native Method)

[23:53:08]W: [Gradle failure report] at java.lang.ClassLoader.defineClass(***@9.0/ClassLoader.java:820)

[23:53:08]W: [Gradle failure report] at java.security.SecureClassLoader.defineClass(***@9.0/SecureClassLoader.java:152)



Which indicates the change to setAccessible also broke Guava, which is used by Gradle. So it's going to be very complicated to even try to fix the issue in those conditions. Anyway, it doesn't seem a good idea to introduce a new exception type. Even if it is semantically a bit problematic, wouldn't make ReflectiveOperationException a subclass of SecurityException an option?





2015-09-11 9:39 GMT+02:00 Alan Bateman <***@oracle.com>:

On 10/09/2015 23:57, Uwe Schindler wrote:

:

I also wanted to ask the question: setAccessible() already throws some checked exceptions, why invent a new one that’s no longer checked? Could it not simply be one already there - one of the checked ones - or a new one just subclassing ReflectiveOperationException? This is completely inconsistent to other core reflection APIs. Why have a totally unexpected new one?



Right, there is compatibility issue here (third item in the "Risks and Assumptions" section of JEP 261 [1]).

The setAccessible methods don't declare any checked exceptions. The only exception they declare is SecurityException. This exception make sense when running with a security manager and the specified permission check fails. This exception is the wrong exception to fail with when the member is not an an exported package as this is nothing to do with the security manager. Sadly, these methods do throw SecurityException when attempted on a constructor of java.lang.Class, I've been trying to dig into the history on this oddity.

Anyway, retrofitting these methods to throw ReflectiveOperationException (or sub-type) isn't really an option because this is a checked exception so it would add a source compatibility issue to the list. For now, the prototype API changes these methods to throw InaccessibleObjectException and we'll have to see the impact of this. I hope we aren't force to change this to throw SecurityException as it's the wrong exception, but less impact compatibility-wise because there is some chance that existing code might handle that already.

-Alan.

[1] http://openjdk.java.net/jeps/261
Uwe Schindler
2015-09-11 08:58:52 UTC
Permalink
Hi,
Post by Uwe Schindler
Post by Uwe Schindler
I also wanted to ask the question: setAccessible() already throws
some
checked exceptions, why invent a new one that’s no longer checked?
Could it not simply be one already there - one of the checked ones -
or a new one just subclassing ReflectiveOperationException? This is
completely inconsistent to other core reflection APIs. Why have a
totally unexpected new one?
Right, there is compatibility issue here (third item in the "Risks and
Assumptions" section of JEP 261 [1]).
The setAccessible methods don't declare any checked exceptions. The
only exception they declare is SecurityException. This exception make
sense when running with a security manager and the specified
permission check fails. This exception is the wrong exception to fail
with when the member is not an an exported package as this is nothing
to do with the security manager. Sadly, these methods do throw
SecurityException when attempted on a constructor of java.lang.Class,
I've been trying to dig into the history on this oddity.
Sorry I missed that. You are right, setAccessible does not throw any checked reflective Exception. I was under the impression it might throw IllegalAccessException or the like, sorry for that. But the issue still persists!

One solution to solve the problem would be to make the java.lang.reflect.InaccessibleObjectException a subclass of SecurityException? This also looks not ideal, but this would fix all code which is well behaved and catches SecurityException on setAccessible. And simple googling shows tons of this.

You may add this to the Javadocs of this method and to the documentation of the Exception, explaining that it is a subclass of SecurityException for backwards compatibility. Are there any other places where java.lang.reflect.InaccessibleObjectException is used?
Post by Uwe Schindler
Anyway, retrofitting these methods to throw
ReflectiveOperationException (or sub-type) isn't really an option
because this is a checked exception so it would add a source
compatibility issue to the list. For now, the prototype API changes
these methods to throw InaccessibleObjectException and we'll have to
see the impact of this. I hope we aren't force to change this to throw
SecurityException as it's the wrong exception, but less impact
compatibility- wise because there is some chance that existing code might handle that already.
I agree, maybe add new exception as described above.
Post by Uwe Schindler
-Alan.
[1] http://openjdk.java.net/jeps/261
Thanks, very helpful. So looks like setAccessible is the only affected method.
Alan Bateman
2015-09-11 12:29:25 UTC
Permalink
Post by Jochen Theodorou
As far as I understood a classloader can have a N modules plus the
unnamed module. I assume a the children of a classloader do not
automatically share modules (or not at all?) with their children.
The "Class loaders" section in the SOTMS document provides a good
summary of the proposed design. The current builds implement this so
that a module is defined to a class loader. You can't some types in a
module defined by one class loader and other types in the same module
defined by a different class loader (how would package private access
work for example).

I'm sure there will be lots of discussion about class loaders in the JSR.
Post by Jochen Theodorou
Do assume right in that there is no way to create a module at runtime?
Next question is about runtime generated classes. And I mean here
classes generated by user code at runtime, as well as proxies, code
for reflection and invokedynamic and all the other bytecode generating
facilities. Will they all go into the unnamed module?
In the proposed design there is a concept of layers of modules. The
EA/prototype builds have API support for this and you'll find tests in
the jdk repo that exercise this API to create configurations and
instantiate them as modules in the run-time.

Supporting dynamic languages and creating dynamic modules is very much
an advanced topic at this time and there isn't support in the exported
API for this. There is of course low-level support in the implementation
and you'll find that Proxy and a few other areas that spin classes at
run-time are using it in the current builds.

At this time, if you are spinning classes the same package/loader of
existing modules then those generated classes will be members of the
module. If you are spinning classes into new packages or different class
loaders that don't have any modules defined to them then they will be in
that loader's unnamed module.
Post by Jochen Theodorou
Assuming they go there (how else can you choose from those N modules),
what access rights to the modules will they have? I assume to
everything that is exported, and for that it does not matter if it is
the same class loader or a parent or any other loader.
Assuming they created in an unnamed module then it's as per the "Unnamed
modules" section of the document.

I see you have other questions about Groovy being a man-in-the-middle. I
don't have time to reply to that now, others have have cycles to engage
on that topic.

-Alan.
Jochen Theodorou
2015-09-11 13:20:31 UTC
Permalink
Am 11.09.2015 14:29, schrieb Alan Bateman:
[...]
Post by Alan Bateman
Supporting dynamic languages and creating dynamic modules is very much
an advanced topic at this time and there isn't support in the exported
API for this. There is of course low-level support in the implementation
and you'll find that Proxy and a few other areas that spin classes at
run-time are using it in the current builds.
I will have a look. But this most certainly means we will have to change
several parts of the code. We use our own class loaders in several
areas, as well as runtime generated classes.
Post by Alan Bateman
At this time, if you are spinning classes the same package/loader of
existing modules then those generated classes will be members of the
module. If you are spinning classes into new packages or different class
loaders that don't have any modules defined to them then they will be in
that loader's unnamed module.
generating classes using a new classloader is the standard for us. That
is imho the case for our callsite caching, but I am very sure for groovy
classes compiled and run at runtime. Since there will be several layers
of code between the place that causes the compilation and execution as
well as the place that does it I also currently see no way to add
ourselves like a layer. It would require to be able to know the caller
class/module or something like this. That is still an unsolved problem
on the JVM for non-JDK code - imho.

bye Jochen
--
Jochen "blackdrag" Theodorou
blog: http://blackdragsview.blogspot.com/
Continue reading on narkive:
Loading...