You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Taher Alkhateeb <sl...@gmail.com> on 2018/05/26 22:33:52 UTC
Re: svn commit: r1832199 - /ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/JavaEventHandler.java
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, <jl...@apache.org> 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);
> }
>
>
Re: svn commit: r1832199 - /ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/JavaEventHandler.java
Posted by Mathieu Lirzin <ma...@nereide.fr>.
Hello Taher,
Taher Alkhateeb <sl...@gmail.com> 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
Re: svn commit: r1832199 -
/ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/JavaEventHandler.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
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, <jl...@apache.org> 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);
>> }
>>
>>