Le 27/05/2018 à 00:33, Taher Alkhateeb a écrit :
So just to make sure I understand this correctly, we're replacing the
old synchronized block with a ConcurrentHashMap using the atomic
function "computeIfAbsent" and then refactored the class loading logic
into a separate method right?
Yes, that's it

Given how critical this piece of code is, wouldn't it be more
appropriate to introduce an integration test and apply it here? I
didn't find any testing feedback on the JIRA.
I don't think we need a test for Java events. I simply tested by hand creating 
an order from ecomseo. It chains processorder:

    <request-map uri="processorder">
        <security https="true" auth="true"/>
        <event type="java" path="org.apache.ofbiz.order.shoppingcart.CheckOutEvents" 
invoke="createOrder"/>
        <response name="sales_order" type="request" value="checkBlackList"/>
        <response name="work_order" type="request" value="checkBlackList"/>
        <response name="purchase_order" type="request" value="clearpocart"/>
        <response name="error" type="view" value="confirm"/>
    </request-map>

If you feel it really needs an integration test, please don't refrain or maybe 
ask Mathieu :)

Jacques


On Thu, May 24, 2018 at 11:42 PM,  <[email protected]> wrote:
Author: jleroux
Date: Thu May 24 20:42:19 2018
New Revision: 1832199

URL: http://svn.apache.org/viewvc?rev=1832199&view=rev
Log:
Improved: Refactor `JavaEventHandler` class
(OFBIZ-10410)

Instead of relying on manual synchronisation, use a concurrent map for caching
loaded classes.
Inline private `invoke` delegate which intent was fuzzy.
Spread arguments when calling `Method::invoke` and `Class::getMethod`.

jleroux: interesting, it's the 1st use in OFBiz of the "::" syntax for calling
methods
(cf. 
https://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-15.13-500)
I note that the string "::" is used in many places as a separator, notably
in caches, as  cache key separator but this should not be confusing.

Thanks: Mathieu Lirzin

Modified:
     
ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/JavaEventHandler.java

Modified: 
ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/JavaEventHandler.java
URL: 
http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/JavaEventHandler.java?rev=1832199&r1=1832198&r2=1832199&view=diff
==============================================================================
--- 
ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/JavaEventHandler.java
 (original)
+++ 
ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/JavaEventHandler.java
 Thu May 24 20:42:19 2018
@@ -19,8 +19,7 @@
  package org.apache.ofbiz.webapp.event;

  import java.lang.reflect.Method;
-import java.util.HashMap;
-import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;

  import javax.servlet.ServletContext;
  import javax.servlet.http.HttpServletRequest;
@@ -40,7 +39,20 @@ public class JavaEventHandler implements

      public static final String module = JavaEventHandler.class.getName();

-    private Map<String, Class<?>> eventClassMap = new HashMap<String, 
Class<?>>();
+    /* Cache for event handler classes. */
+    private ConcurrentHashMap<String, Class<?>> classes = new 
ConcurrentHashMap<>();
+
+    /* Return class corresponding to path or null. */
+    private static Class<?> loadClass(String path) {
+        try {
+            ClassLoader l = Thread.currentThread().getContextClassLoader();
+            return l.loadClass(path);
+        } catch (ClassNotFoundException e) {
+            Debug.logError(e, "Error loading class with name: "+ path
+                    + ", will not be able to run event...", module);
+            return null;
+        }
+    }

      /**
       * @see 
org.apache.ofbiz.webapp.event.EventHandler#init(javax.servlet.ServletContext)
@@ -51,56 +63,29 @@ public class JavaEventHandler implements
      /**
       * @see 
org.apache.ofbiz.webapp.event.EventHandler#invoke(ConfigXMLReader.Event, 
ConfigXMLReader.RequestMap, javax.servlet.http.HttpServletRequest, 
javax.servlet.http.HttpServletResponse)
       */
-    public String invoke(Event event, RequestMap requestMap, 
HttpServletRequest request, HttpServletResponse response) throws 
EventHandlerException {
-        Class<?> eventClass = this.eventClassMap.get(event.path);
-
-        if (eventClass == null) {
-            synchronized (this) {
-                eventClass = this.eventClassMap.get(event.path);
-                if (eventClass == null) {
-                    try {
-                        ClassLoader loader = 
Thread.currentThread().getContextClassLoader();
-                        eventClass = loader.loadClass(event.path);
-                    } catch (ClassNotFoundException e) {
-                        Debug.logError(e, "Error loading class with name: " + event.path 
+ ", will not be able to run event...", module);
-                    }
-                    if (eventClass != null) {
-                        eventClassMap.put(event.path, eventClass);
-                    }
-                }
-            }
-        }
-        if (Debug.verboseOn()) Debug.logVerbose("[Set path/method]: " + event.path + 
" / " + event.invoke, module);
-
-        Class<?>[] paramTypes = new Class<?>[] {HttpServletRequest.class, 
HttpServletResponse.class};
-
+    public String invoke(Event event, RequestMap requestMap,
+            HttpServletRequest request, HttpServletResponse response)
+                    throws EventHandlerException {
+        Class<?> k = classes.computeIfAbsent(event.path, 
JavaEventHandler::loadClass);
          if (Debug.verboseOn()) Debug.logVerbose("*[[Event invocation]]*", 
module);
-        Object[] params = new Object[] {request, response};
-
-        return invoke(event.path, event.invoke, eventClass, paramTypes, 
params, event.transactionTimeout);
-    }
-
-    private String invoke(String eventPath, String eventMethod, Class<?> eventClass, 
Class<?>[] paramTypes, Object[] params, int transactionTimeout) throws 
EventHandlerException {
-        boolean beganTransaction = false;
-        if (eventClass == null) {
-            throw new EventHandlerException("Error invoking event, the class " + 
eventPath + " was not found");
+        if (k == null) {
+            throw new EventHandlerException("Error invoking event, the class "
+                                            + event.path + " was not found");
          }
-        if (eventPath == null || eventMethod == null) {
+        if (event.path == null || event.invoke == null) {
              throw new EventHandlerException("Invalid event method or path; call 
initialize()");
          }

-        if (Debug.verboseOn()) Debug.logVerbose("[Processing]: JAVA Event", 
module);
+        if (Debug.verboseOn()) Debug.logVerbose("[Processing]: Java Event", 
module);
+        boolean began = false;
          try {
-            if (transactionTimeout > 0) {
-                beganTransaction = TransactionUtil.begin(transactionTimeout);
-            } else {
-                beganTransaction = TransactionUtil.begin();
-            }
-            Method m = eventClass.getMethod(eventMethod, paramTypes);
-            String eventReturn = (String) m.invoke(null, params);
-
-            if (Debug.verboseOn()) Debug.logVerbose("[Event Return]: " + 
eventReturn, module);
-            return eventReturn;
+            int timeout = Integer.max(event.transactionTimeout, 0);
+            began = TransactionUtil.begin(timeout);
+            Method m = k.getMethod(event.invoke, HttpServletRequest.class,
+                                   HttpServletResponse.class);
+            String ret = (String) m.invoke(null, request, response);
+            if (Debug.verboseOn()) Debug.logVerbose("[Event Return]: " + ret, 
module);
+            return ret;
          } catch (java.lang.reflect.InvocationTargetException e) {
              Throwable t = e.getTargetException();

@@ -116,7 +101,7 @@ public class JavaEventHandler implements
              throw new EventHandlerException("Problems processing event: " + 
e.toString(), e);
          } finally {
              try {
-                TransactionUtil.commit(beganTransaction);
+                TransactionUtil.commit(began);
              } catch (GenericTransactionException e) {
                  Debug.logError(e, module);
              }



Reply via email to