Re: svn commit: r1832199 - /ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/JavaEventHandler.java

[taher]

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?

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.

On Thu, May 24, 2018 at 11:42 PM,  <[hidden email]> 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);
>              }
>
>

[Jacques Le Roux]

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,  <[hidden email]> 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);
>>               }
>>
>>

[Mathieu Lirzin]

Hello Taher,

Taher Alkhateeb <[hidden email]> writes:

> 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?

That's correct. ‘ConcurrentHashMap::computeIfAbsent’ ensures that the
method invokation is done atomically [1].

> 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.

Indeed being critical makes it desirable to provide a test.  However
concurrency issues are hardly testable in practice since the execution
order is nondeterministic.  I think we can't do better than reasoning
about the actual concurrency model.  Are you puzzled by a specific
scenario?

Thanks.

[1] https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentHashMap.html#computeIfAbsent-K-java.util.function.Function-

--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37