Master fix #42
Master fix #42
Conversation
|
Created #44 to track this. |
ryanemerson
force-pushed
the
fix_master
branch
3 times, most recently
from
92a9cd1
to
e37d5dc
Compare
| private UnaryOperator<Key> checkForCloneMethod(final Class<?> declType, final Class<?> returnType) | ||
| { | ||
| System.out.printf("Call checkForCloneMethod(%s,%s)%n", declType, returnType); | ||
| final Method method = doPrivileged(new PrivilegedAction<Method>() |
There was a problem hiding this comment.
Can you explain how this fixes it? I thought the issue was that that it checked all the various classes for clone method in the reflection list and found an invalid one even though this would never invoke it, is that not true? Or are we working around that by using reflection methods directly instead of MethodHandle which seem to be stricter? To me this seems like a very brittle "fix".
Can you elaborate? Thanks.
There was a problem hiding this comment.
Why not just use a lambda?
Didn't work with lambdas. I got some error when using lambda, so I decided to use anonymous inner class which works just as fine.
Can you explain how this fixes it? I thought the issue was that that it checked all the various classes for clone method in the reflection list and found an invalid one even though this would never invoke it, is that not true? Or are we working around that by using reflection methods directly instead of MethodHandle which seem to be stricter? To me this seems like a very brittle "fix".
We're just doing the same logic that Elytron does but instead of using MethodHandles, use reflection. The use of MethodHandles was flat out rejected. Reflection works, but this logic will only kick in with particular keys that fit the bill, e.g. https://github.com/galderz/mendrugo/blob/master/elytron-cloning/src/main/java/org/example/elytron/Main.java#L189 or https://github.com/galderz/mendrugo/blob/master/elytron-cloning/src/main/java/org/example/elytron/Main.java#L138. The logic that reflection or methodhandles use only kicks in very specific scenarios, e.g. destroy() needs to be overriden and other stuff that the Elytron code requires (can't remember now). Now, whether Infinispan is affected by this or not is unknown to me. It depends on the types of keys it uses which I've no idea of. If any of those would be used, you'd need to register them for reflection for things to work. What these changes do is allow all of this to work.
If you have more doubts, I'd suggest you check out the demo code in here, run it, play with it and see how it all fits together.
There was a problem hiding this comment.
It wasn't doubts as much as I was curious why reflection works but MethodHandles do. And the fact that I would think Graal would eventually add the same checking for reflection that MethodHandles has. But that is why I was curious as to why it actually worked.
There was a problem hiding this comment.
I've not looked at the MethodHandles code, but reflection works but capturing each class/method/constructor to be accessed, generates a proxy for each and eventually proxy instances are substituted by JDK internal reflection FieldAccessor/ConstructorAccessor/MethodAccessor instances.
| if (method == null) | ||
| return null; | ||
|
|
||
| return new UnaryOperator<Key>() |
| @Substitute | ||
| private UnaryOperator<Key> checkForCloneMethod(final Class<?> declType, final Class<?> returnType) | ||
| { | ||
| System.out.printf("Call checkForCloneMethod(%s,%s)%n", declType, returnType); |
There was a problem hiding this comment.
We shouldn't need the printf anymore.
There was a problem hiding this comment.
Good spot. Removed and I have updated the class to use our style conventions.
ryanemerson
force-pushed
the
fix_master
branch
from
e37d5dc
to
9255860
Compare
…mbination infinispan#44 Co-authored-by: Galder Zamarreño <galder@redhat.com>
ryanemerson
force-pushed
the
fix_master
branch
from
9255860
to
11a06e7
Compare
ryanemerson
force-pushed
the
fix_master
branch
from
ed45588
to
e3bbcbb
Compare
|
Integrated into master, thanks @ryanemerson ! |
@wburns This fixes a compilation issue with the latest Infinispan master, however I'm now getting the following: