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