From dbb3d08ffabef14ef6caf0ddda24e1e64770b965 Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Sun, 21 Jan 2024 09:53:05 +0100 Subject: [PATCH 1/4] fix: Assign method call listeners directly to the proxy instance --- .../proxy/HasMethodCallListeners.java | 35 +++++ .../io/appium/java_client/proxy/Helpers.java | 12 +- .../appium/java_client/proxy/Interceptor.java | 25 ++- .../proxy/ProxyListenersContainer.java | 145 ------------------ 4 files changed, 55 insertions(+), 162 deletions(-) create mode 100644 src/main/java/io/appium/java_client/proxy/HasMethodCallListeners.java delete mode 100644 src/main/java/io/appium/java_client/proxy/ProxyListenersContainer.java diff --git a/src/main/java/io/appium/java_client/proxy/HasMethodCallListeners.java b/src/main/java/io/appium/java_client/proxy/HasMethodCallListeners.java new file mode 100644 index 000000000..b5807f71b --- /dev/null +++ b/src/main/java/io/appium/java_client/proxy/HasMethodCallListeners.java @@ -0,0 +1,35 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * See the NOTICE file distributed with this work for additional + * information regarding copyright ownership. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.appium.java_client.proxy; + +public interface HasMethodCallListeners { + /** + * The setter is dynamically created by ByteBuddy to store + * method call listeners on the instrumented proxy instance. + * + * @param methodCallListeners Array of method call listeners assigned to the proxy instance. + */ + void setMethodCallListeners(MethodCallListener[] methodCallListeners); + + /** + * The getter is dynamically created by ByteBuddy to access + * method call listeners on the instrumented proxy instance. + * + * @return Array of method call listeners assigned the proxy instance. + */ + MethodCallListener[] getMethodCallListeners(); +} diff --git a/src/main/java/io/appium/java_client/proxy/Helpers.java b/src/main/java/io/appium/java_client/proxy/Helpers.java index 9543073a8..1b0c7a6d4 100644 --- a/src/main/java/io/appium/java_client/proxy/Helpers.java +++ b/src/main/java/io/appium/java_client/proxy/Helpers.java @@ -19,7 +19,9 @@ import com.google.common.base.Preconditions; import net.bytebuddy.ByteBuddy; import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.modifier.Visibility; import net.bytebuddy.dynamic.loading.ClassLoadingStrategy; +import net.bytebuddy.implementation.FieldAccessor; import net.bytebuddy.implementation.MethodDelegation; import net.bytebuddy.matcher.ElementMatcher; import net.bytebuddy.matcher.ElementMatchers; @@ -118,16 +120,18 @@ public static T createProxy( .subclass(cls) .method(extraMethodMatcher == null ? matcher : matcher.and(extraMethodMatcher)) .intercept(MethodDelegation.to(Interceptor.class)) + // https://github.com/raphw/byte-buddy/blob/2caef35c172897cbdd21d163c55305a64649ce41/byte-buddy-dep/src/test/java/net/bytebuddy/ByteBuddyTutorialExamplesTest.java#L346 + .defineField("methodCallListeners", MethodCallListener[].class, Visibility.PRIVATE) + .implement(HasMethodCallListeners.class).intercept(FieldAccessor.ofBeanProperty()) .make() .load(ClassLoader.getSystemClassLoader(), ClassLoadingStrategy.Default.WRAPPER) .getLoaded() .asSubclass(cls); try { - return ProxyListenersContainer.getInstance().setListeners( - cls.cast(proxy.getConstructor(constructorArgTypes).newInstance(constructorArgs)), - listeners - ); + T result = cls.cast(proxy.getConstructor(constructorArgTypes).newInstance(constructorArgs)); + ((HasMethodCallListeners) result).setMethodCallListeners(listeners.toArray(MethodCallListener[]::new)); + return result; } catch (SecurityException | ReflectiveOperationException e) { throw new IllegalStateException(String.format("Unable to create a proxy of %s", cls.getName()), e); } diff --git a/src/main/java/io/appium/java_client/proxy/Interceptor.java b/src/main/java/io/appium/java_client/proxy/Interceptor.java index b8417e14b..1ba381b58 100644 --- a/src/main/java/io/appium/java_client/proxy/Interceptor.java +++ b/src/main/java/io/appium/java_client/proxy/Interceptor.java @@ -25,7 +25,6 @@ import org.slf4j.LoggerFactory; import java.lang.reflect.Method; -import java.util.Collection; import java.util.UUID; import java.util.concurrent.Callable; @@ -53,12 +52,12 @@ public static Object intercept( @AllArguments Object[] args, @SuperCall Callable callable ) throws Throwable { - Collection listeners = ProxyListenersContainer.getInstance().getListeners(self); - if (listeners.isEmpty()) { + var listeners = ((HasMethodCallListeners) self).getMethodCallListeners(); + if (listeners.length == 0) { return callable.call(); } - listeners.forEach(listener -> { + for (var listener : listeners) { try { listener.beforeCall(self, method, args); } catch (NotImplementedException e) { @@ -68,11 +67,11 @@ public static Object intercept( self.getClass().getName(), method.getName(), e ); } - }); + } - final UUID noResult = UUID.randomUUID(); - Object result = noResult; - for (MethodCallListener listener : listeners) { + final UUID notAssigned = UUID.randomUUID(); + Object result = notAssigned; + for (var listener : listeners) { try { result = listener.call(self, method, args, callable); break; @@ -87,11 +86,11 @@ public static Object intercept( throw e; } } - if (noResult.equals(result)) { + if (notAssigned == result) { try { result = callable.call(); } catch (Exception e) { - for (MethodCallListener listener : listeners) { + for (var listener : listeners) { try { return listener.onError(self, method, args, e); } catch (NotImplementedException ignore) { @@ -102,8 +101,8 @@ public static Object intercept( } } - final Object endResult = result == noResult ? null : result; - listeners.forEach(listener -> { + final Object endResult = result == notAssigned ? null : result; + for (var listener : listeners) { try { listener.afterCall(self, method, args, endResult); } catch (NotImplementedException e) { @@ -113,7 +112,7 @@ public static Object intercept( self.getClass().getName(), method.getName(), e ); } - }); + } return endResult; } } diff --git a/src/main/java/io/appium/java_client/proxy/ProxyListenersContainer.java b/src/main/java/io/appium/java_client/proxy/ProxyListenersContainer.java deleted file mode 100644 index 4d93ebefb..000000000 --- a/src/main/java/io/appium/java_client/proxy/ProxyListenersContainer.java +++ /dev/null @@ -1,145 +0,0 @@ -/* - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * See the NOTICE file distributed with this work for additional - * information regarding copyright ownership. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package io.appium.java_client.proxy; - -import lombok.AllArgsConstructor; -import lombok.Getter; -import lombok.Setter; - -import java.lang.ref.WeakReference; -import java.time.Duration; -import java.util.Collection; -import java.util.Collections; -import java.util.LinkedList; -import java.util.List; -import java.util.Timer; -import java.util.TimerTask; -import java.util.concurrent.Semaphore; - -class ProxyListenersContainer { - private static final Duration LISTENERS_CLEANUP_INTERVAL = Duration.ofMinutes(1); - - private static ProxyListenersContainer INSTANCE; - - public static synchronized ProxyListenersContainer getInstance() { - if (INSTANCE == null) { - INSTANCE = new ProxyListenersContainer(); - } - return INSTANCE; - } - - private ProxyListenersContainer() { - var task = new TimerTask() { - @Override - public void run() { - getListeners(null); - } - }; - // Listeners are cleaned up lazily, e.g. every time getListeners API - // is called we also remove garbage-collected items. Although, due to an - // unpredictable nature of the garbage collector and no guarantees about the - // frequency of getListeners API calls we schedule the below loop to be executed every - // minute, and make sure there are no extra references to obsolete listeners - new Timer().scheduleAtFixedRate(task, 0, LISTENERS_CLEANUP_INTERVAL.toMillis()); - } - - private final Semaphore listenersGuard = new Semaphore(1); - private final List, Collection>> listenerPairs = new LinkedList<>(); - - @Getter - @AllArgsConstructor - private static class Pair { - private final K key; - @Setter - private V value; - } - - /** - * Assign listeners for the particular proxied instance. - * - * @param proxyInstance The proxied instance. - * @param listeners Collection of listeners. - * @return The same given instance. - */ - public T setListeners(T proxyInstance, Collection listeners) { - try { - listenersGuard.acquire(); - } catch (InterruptedException e) { - throw new RuntimeException(e); - } - try { - int i = 0; - boolean wasInstancePresent = false; - while (i < listenerPairs.size()) { - var pair = listenerPairs.get(i); - Object key = pair.getKey().get(); - if (key == null) { - // The instance has been garbage-collected - listenerPairs.remove(i); - continue; - } - - if (key == proxyInstance) { - pair.setValue(List.copyOf(listeners)); - wasInstancePresent = true; - } - i++; - } - if (!wasInstancePresent) { - listenerPairs.add(new Pair<>(new WeakReference<>(proxyInstance), List.copyOf(listeners))); - } - } finally { - listenersGuard.release(); - } - return proxyInstance; - } - - /** - * Fetches listeners for the particular proxied instance. - * - * @param proxyInstance The proxied instance. - */ - public Collection getListeners(Object proxyInstance) { - try { - listenersGuard.acquire(); - } catch (InterruptedException e) { - throw new RuntimeException(e); - } - try { - int i = 0; - Collection result = Collections.emptyList(); - while (i < listenerPairs.size()) { - var pair = listenerPairs.get(i); - Object key = pair.getKey().get(); - if (key == null) { - // The instance has been garbage-collected - listenerPairs.remove(i); - continue; - } - - if (key == proxyInstance) { - result = pair.getValue(); - } - i++; - } - return result; - } finally { - listenersGuard.release(); - } - } - -} From 174a717bd72aa94aa33bdca156f941fb677a07c4 Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Sun, 21 Jan 2024 10:35:24 +0100 Subject: [PATCH 2/4] Tune --- .../io/appium/java_client/proxy/Interceptor.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/main/java/io/appium/java_client/proxy/Interceptor.java b/src/main/java/io/appium/java_client/proxy/Interceptor.java index 1ba381b58..a9271c9bb 100644 --- a/src/main/java/io/appium/java_client/proxy/Interceptor.java +++ b/src/main/java/io/appium/java_client/proxy/Interceptor.java @@ -30,13 +30,16 @@ public class Interceptor { private static final Logger LOGGER = LoggerFactory.getLogger(Interceptor.class); + private static final UUID NOT_ASSIGNED = UUID.randomUUID(); private Interceptor() { } /** * A magic method used to wrap public method calls in classes - * patched by ByteBuddy and acting as proxies. + * patched by ByteBuddy and acting as proxies. The performance + * of this method is mission-critical as it gets called upon + * every invocation of any method of the proxied class. * * @param self The reference to the original instance. * @param method The reference to the original method. @@ -53,7 +56,7 @@ public static Object intercept( @SuperCall Callable callable ) throws Throwable { var listeners = ((HasMethodCallListeners) self).getMethodCallListeners(); - if (listeners.length == 0) { + if (listeners == null || listeners.length == 0) { return callable.call(); } @@ -69,8 +72,7 @@ public static Object intercept( } } - final UUID notAssigned = UUID.randomUUID(); - Object result = notAssigned; + Object result = NOT_ASSIGNED; for (var listener : listeners) { try { result = listener.call(self, method, args, callable); @@ -86,7 +88,7 @@ public static Object intercept( throw e; } } - if (notAssigned == result) { + if (NOT_ASSIGNED == result) { try { result = callable.call(); } catch (Exception e) { @@ -101,7 +103,7 @@ public static Object intercept( } } - final Object endResult = result == notAssigned ? null : result; + final Object endResult = result == NOT_ASSIGNED ? null : result; for (var listener : listeners) { try { listener.afterCall(self, method, args, endResult); From 30d57a6da8b12c472bb8fbe9cfac4a53c9305b25 Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Sun, 21 Jan 2024 13:36:47 +0100 Subject: [PATCH 3/4] performance --- .../appium/java_client/proxy/Interceptor.java | 24 ++++++++++++------- .../java_client/proxy/MethodCallListener.java | 14 +++++------ 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/src/main/java/io/appium/java_client/proxy/Interceptor.java b/src/main/java/io/appium/java_client/proxy/Interceptor.java index a9271c9bb..f4ece1668 100644 --- a/src/main/java/io/appium/java_client/proxy/Interceptor.java +++ b/src/main/java/io/appium/java_client/proxy/Interceptor.java @@ -25,12 +25,12 @@ import org.slf4j.LoggerFactory; import java.lang.reflect.Method; -import java.util.UUID; import java.util.concurrent.Callable; +import static io.appium.java_client.proxy.MethodCallListener.UNSET; + public class Interceptor { private static final Logger LOGGER = LoggerFactory.getLogger(Interceptor.class); - private static final UUID NOT_ASSIGNED = UUID.randomUUID(); private Interceptor() { } @@ -72,29 +72,37 @@ public static Object intercept( } } - Object result = NOT_ASSIGNED; + Object result = UNSET; for (var listener : listeners) { try { result = listener.call(self, method, args, callable); - break; + if (result != UNSET) { + break; + } } catch (NotImplementedException e) { // ignore } catch (Exception e) { try { - return listener.onError(self, method, args, e); + result = listener.onError(self, method, args, e); + if (result != UNSET) { + return result; + } } catch (NotImplementedException ignore) { // ignore } throw e; } } - if (NOT_ASSIGNED == result) { + if (UNSET == result) { try { result = callable.call(); } catch (Exception e) { for (var listener : listeners) { try { - return listener.onError(self, method, args, e); + result = listener.onError(self, method, args, e); + if (result != UNSET) { + return result; + } } catch (NotImplementedException ignore) { // ignore } @@ -103,7 +111,7 @@ public static Object intercept( } } - final Object endResult = result == NOT_ASSIGNED ? null : result; + final Object endResult = result == UNSET ? null : result; for (var listener : listeners) { try { listener.afterCall(self, method, args, endResult); diff --git a/src/main/java/io/appium/java_client/proxy/MethodCallListener.java b/src/main/java/io/appium/java_client/proxy/MethodCallListener.java index f0cc1be7a..12b9a8d05 100644 --- a/src/main/java/io/appium/java_client/proxy/MethodCallListener.java +++ b/src/main/java/io/appium/java_client/proxy/MethodCallListener.java @@ -17,9 +17,11 @@ package io.appium.java_client.proxy; import java.lang.reflect.Method; +import java.util.UUID; import java.util.concurrent.Callable; public interface MethodCallListener { + UUID UNSET = UUID.randomUUID(); /** * The callback to be invoked before any public method of the proxy is called. @@ -30,9 +32,7 @@ public interface MethodCallListener { * @param method Method to be called * @param args Array of method arguments */ - default void beforeCall(Object obj, Method method, Object[] args) { - throw new NotImplementedException(); - } + default void beforeCall(Object obj, Method method, Object[] args) {} /** * Override this callback in order to change/customize the behavior @@ -48,7 +48,7 @@ default void beforeCall(Object obj, Method method, Object[] args) { * @return The type of the returned result should be castable to the returned type of the original method. */ default Object call(Object obj, Method method, Object[] args, Callable original) throws Throwable { - throw new NotImplementedException(); + return UNSET; } /** @@ -60,9 +60,7 @@ default Object call(Object obj, Method method, Object[] args, Callable origin * @param method Method to be called * @param args Array of method arguments */ - default void afterCall(Object obj, Method method, Object[] args, Object result) { - throw new NotImplementedException(); - } + default void afterCall(Object obj, Method method, Object[] args, Object result) {} /** * The callback to be invoked if a public method or its @@ -77,6 +75,6 @@ default void afterCall(Object obj, Method method, Object[] args, Object result) * type of the returned argument could be cast to the returned type of the original method. */ default Object onError(Object obj, Method method, Object[] args, Throwable e) throws Throwable { - throw new NotImplementedException(); + return UNSET; } } From c1fa684478cd7fa282b58f5762ef3fe2a28befb2 Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Sun, 21 Jan 2024 13:46:03 +0100 Subject: [PATCH 4/4] Fix style --- .../io/appium/java_client/proxy/MethodCallListener.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/main/java/io/appium/java_client/proxy/MethodCallListener.java b/src/main/java/io/appium/java_client/proxy/MethodCallListener.java index 12b9a8d05..7dfb5b299 100644 --- a/src/main/java/io/appium/java_client/proxy/MethodCallListener.java +++ b/src/main/java/io/appium/java_client/proxy/MethodCallListener.java @@ -32,7 +32,8 @@ public interface MethodCallListener { * @param method Method to be called * @param args Array of method arguments */ - default void beforeCall(Object obj, Method method, Object[] args) {} + default void beforeCall(Object obj, Method method, Object[] args) { + } /** * Override this callback in order to change/customize the behavior @@ -60,7 +61,8 @@ default Object call(Object obj, Method method, Object[] args, Callable origin * @param method Method to be called * @param args Array of method arguments */ - default void afterCall(Object obj, Method method, Object[] args, Object result) {} + default void afterCall(Object obj, Method method, Object[] args, Object result) { + } /** * The callback to be invoked if a public method or its