Wrap Event Listener Registration #3619

gabizou · 2022-02-01T06:59:46Z

The end of Reflection

The main goal of this PR is to eliminate usages of Java's Reflection API due to the intrinsic nature of Reflection being finicky with our lack of control over ClassLoaders. There are specific cases where classes may not be available and due to the nature of the JVM, it's acceptable for some classes to be missing since the exception will only be triggered by code path explicitly. This explicit access is normally triggered by compiled code, but can be implicitly triggered by Reflection.

The Solution

Instead of triggering class loads by Reflection, we will instead use ASM ClassReaders to perform the work we need to do. In some cases, this is as simple as a ClassVisitor that analyzes the methods declared on the class. In other cases, it's finding any Listeners available on a Class.

Fixes #3589 #3597 #3574 #3548 #3532

zml2008 · 2022-02-04T03:15:16Z

src/main/java/org/spongepowered/common/event/manager/InvokeEventListenerFactory.java

-                this.method.invoke(this.handle, event);
-            }
+        public void handle(final Event event) throws Exception {
+//            if (this.filter != null) {


I don't think this can reasonably come back, maybe just delete this whole factory?

Having reviewed it in SF 1.12, it doesn't even seem to have been used even then. Deleting!

src/main/java/org/spongepowered/common/event/manager/ListenerClassVisitor.java

zml2008 · 2022-02-04T03:17:49Z

src/main/java/org/spongepowered/common/event/manager/ListenerClassVisitor.java

+            for (int i = 0; i < type.length; i++) {
+                parameters[i] = new ListenerParameter(this.discoveredMethod, type[i]);
+            }
+            // TODO - figure out a cleaner way to do this, this works for:


maybe take a look at ASM's SignatureReader? it should be able to capture the applicable type, and Geantyref has methods to create a runtime reflection Type ourselves

That's fair, I did read briefly about using SignatureReader, but didn't connect the dots on making it work (code written at 2am isn't always the sanest).

I did take advantage of Geantyref's TypeFactory in a few places already for creating Type instances as necessary.

Rewrote with SignatureVisitor on the parameter

Avoids game crashes for seemingly client sided methods. Client sided methods can be included in class bytecode, but ultimately it's up to the developer to exclude it properly. In our case, we don't have control over it normally, so we'll have to expect any client side code to trigger some RuntimeExceptions. Signed-off-by: Gabriel Harris-Rouquette <gabizou@me.com>

The end goal is to replace usage of Java Reflection API that can be prone to classloading exceptions/errors that end up crashing or throwing errors. The major problem with reflection is the lack of clear control over ClassLoaders and their ability to throw any arbitrary error. In retrospect, we can now safely scan entire classes for specific methods that we care about without triggering these classes to be loaded. Likewise, as a test, we can assure that a defined class with an impossible to use method will still be correctly covered for our purposes. Signed-off-by: Gabriel Harris-Rouquette <gabizou@me.com>

zml2008 · 2022-02-07T02:15:21Z

src/main/java/org/spongepowered/common/event/filter/FilterGenerator.java

-                    Object obj = FilterGenerator.filterFromAnnotation(anno.annotationType());
+                final List<ParameterFilterDelegate> paramFilters = new ArrayList<>();
+                for (final ListenerClassVisitor.ListenerAnnotation anno : param.annotations()) {
+                    final Object obj = FilterGenerator.filterFromAnnotation(Class.forName(anno.type().getClassName()));


This should probably use method.declaringClass().getClassLoader() to look up the annotation class name

zml2008 · 2022-02-07T02:16:37Z

src/main/java/org/spongepowered/common/event/filter/delegate/GetValueFilterSourceDelegate.java

-            final Parameter param = params[i];
-            if (!ValueContainer.class.isAssignableFrom(param.getType())) {
+            final ListenerClassVisitor.ListenerParameter param = params[i];
+            if (!ValueContainer.class.isAssignableFrom(Class.forName(param.type().getClassName()))) {


should use the declaring class's class loader for this as well

zml2008 · 2022-02-07T02:17:15Z

src/main/java/org/spongepowered/common/event/filter/delegate/GetterFilterSourceDelegate.java

+        final Class<?> targetType = Class.forName(params[paramIdx].type().getClassName());
+        final Class<?> eventClass = Class.forName(params[0].type().getClassName());


declaring class's loader, just in case that's different

zml2008 · 2022-02-07T02:19:30Z

src/main/java/org/spongepowered/common/event/manager/ListenerClassVisitor.java

+        }
+
+        public Class<?> clazz() throws ClassNotFoundException {
+            return Class.forName(this.type.getClassName());


should use method's declaring class

zml2008 · 2022-02-07T02:20:00Z

src/main/java/org/spongepowered/common/event/manager/ListenerClassVisitor.java

+
+        public java.lang.reflect.Type genericType() throws ClassNotFoundException {
+            if (this.genericType == null) {
+                return TypeFactory.parameterizedClass(Class.forName(this.type.getClassName()));


this.clazz()

zml2008 · 2022-02-07T02:20:15Z

src/main/java/org/spongepowered/common/event/manager/ListenerClassVisitor.java

+                return TypeFactory.parameterizedClass(Class.forName(this.type.getClassName()));
+            }
+            final java.lang.reflect.Type generic = TypeFactory.parameterizedClass(Class.forName(this.genericType.getClassName()));
+            return TypeFactory.parameterizedClass(Class.forName(this.type.getClassName()), generic);


this.clazz()

zml2008 · 2022-02-07T02:20:41Z

src/main/java/org/spongepowered/common/event/manager/ListenerClassVisitor.java

+            AnnotationFormatException {
+            this.values.put(name, value);
+            this.annotation = TypeFactory.annotation(
+                (Class<? extends Annotation>) Class.forName(type.getClassName()), this.values);


need to pas the declaring class's classloader through here

This was brutal awakening of how much reflection is relied on to create all sorts of classes. Fortunately, the brunt of the changes are to do with figuring out how to use ASM visitors to do the discovery work. It is now unit tested to ensure classes that can be triggered with reflection for failure, will not fail. An advantage of this is now we are assured the only class loading failures would be breaking changes to our own API classes. Signed-off-by: Gabriel Harris-Rouquette <gabizou@me.com>

Signed-off-by: Gabriel Harris-Rouquette <gabizou@me.com>

gabizou force-pushed the api8/wrap-event-listeners branch 2 times, most recently from 5347c54 to 81c164f Compare February 3, 2022 07:55

gabizou requested review from zml2008, dualspiral and Zidane February 3, 2022 11:12

zml2008 requested changes Feb 4, 2022

View reviewed changes

gabizou force-pushed the api8/wrap-event-listeners branch from 14c9551 to e23feea Compare February 6, 2022 16:23

gabizou added 2 commits February 6, 2022 08:24

gabizou force-pushed the api8/wrap-event-listeners branch from e23feea to e52e553 Compare February 6, 2022 16:24

gabizou requested a review from zml2008 February 6, 2022 16:27

zml2008 requested changes Feb 7, 2022

View reviewed changes

gabizou force-pushed the api8/wrap-event-listeners branch from e52e553 to 5506503 Compare February 8, 2022 02:01

gabizou requested a review from zml2008 February 8, 2022 02:02

Remove unused legacy Invoker factory

5368fe2

Signed-off-by: Gabriel Harris-Rouquette <gabizou@me.com>

gabizou merged commit 5368fe2 into api-8 Feb 8, 2022

gabizou deleted the api8/wrap-event-listeners branch February 8, 2022 03:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrap Event Listener Registration #3619

Wrap Event Listener Registration #3619

gabizou commented Feb 1, 2022 •

edited

zml2008 Feb 4, 2022

gabizou Feb 4, 2022

zml2008 Feb 4, 2022

gabizou Feb 4, 2022

gabizou Feb 6, 2022

zml2008 Feb 7, 2022

zml2008 Feb 7, 2022

zml2008 Feb 7, 2022

zml2008 Feb 7, 2022

zml2008 Feb 7, 2022

zml2008 Feb 7, 2022

zml2008 Feb 7, 2022

		final Class<?> targetType = Class.forName(params[paramIdx].type().getClassName());
		final Class<?> eventClass = Class.forName(params[0].type().getClassName());

Wrap Event Listener Registration #3619

Wrap Event Listener Registration #3619

Conversation

gabizou commented Feb 1, 2022 • edited

The end of Reflection

The Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabizou commented Feb 1, 2022 •

edited