Re: svn commit: r1533839 - in /ofbiz/trunk: ./ framework/base/src/org/ofbiz/b
Locked
Re: svn commit: r1533839 - in /ofbiz/trunk: ./ framework/base/src/org/ofbiz/b
 
|
Jacques,
what was the rationale behind your design decision to add the portOffset field to the ClassLoaderContainer? Why ClassLoaderContainer? It doesn't make much sense to me... Jacopo On Oct 20, 2013, at 12:07 AM, [hidden email] wrote: > Author: jleroux > Date: Sat Oct 19 22:07:35 2013 > New Revision: 1533839 > > URL: http://svn.apache.org/r1533839 > Log: > Set different ports for testing in a CI environment (e.g. Jenkins) https://issues.apache.org/jira/browse/OFBIZ-4794 > > *What's for?* > Finally, OOTB the port offset is interesting in 2 cases: > # To eventually run Sonar on OFBiz sources https://issues.apache.org/jira/browse/INFRA-3590 > # To simultaneously run the demos without having to change much things, just one parameter (this will need to have the same commited in R13.07... to be discussed...) > > Of course, It can be used for other purposes outside of the ASF context... > > *How it's done?* > Basically, I just added a portoffset integer parameter to 3 ant targets (of course this JVM param can also be used in a Java call from command line): start, start-pos, start-both, > > The portoffset parameter is set to zero by default in ClassLoaderContainer.java. > If a value is passed, it's then grabed by Config.java and passed to ClassLoaderContainer.java, where it's set to this value. > > The portoffset is then used to change in one shoot all the default ports values defined (in config or hardcoded), and the admin port value > This is done in following classes: > CatalinaContainer > NamingServiceContainer > ServiceEngine through ServiceLocation > RmiServiceContainer > XMLRPCClientEngine > XmlRpcTests > > To avoid hardcoding locations in services definitions, I also created 2 service-locations: main-http and main-soap > > *To summarize:* these changes will be used to run "ant start" with a portoffset value which will take care of all ports values, including admin port. For running R12.04 demo as stable we will still need to create a patch for the last time. R13.07 could have a similar patch applied since it's not yet released... > > Modified: > ofbiz/trunk/build.xml > ofbiz/trunk/framework/base/src/org/ofbiz/base/container/ClassLoaderContainer.java > ofbiz/trunk/framework/base/src/org/ofbiz/base/container/NamingServiceContainer.java > ofbiz/trunk/framework/catalina/src/org/ofbiz/catalina/container/CatalinaContainer.java > ofbiz/trunk/framework/common/servicedef/services_test.xml > ofbiz/trunk/framework/service/config/serviceengine.xml > ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceEngine.java > ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceLocation.java > ofbiz/trunk/framework/service/src/org/ofbiz/service/engine/XMLRPCClientEngine.java > ofbiz/trunk/framework/service/src/org/ofbiz/service/rmi/RmiServiceContainer.java > ofbiz/trunk/framework/service/src/org/ofbiz/service/test/XmlRpcTests.java > ofbiz/trunk/framework/start/src/org/ofbiz/base/start/Config.java > > Modified: ofbiz/trunk/build.xml > URL: http://svn.apache.org/viewvc/ofbiz/trunk/build.xml?rev=1533839&r1=1533838&r2=1533839&view=diff > ============================================================================== > --- ofbiz/trunk/build.xml (original) > +++ ofbiz/trunk/build.xml Sat Oct 19 22:07:35 2013 > @@ -408,11 +408,13 @@ under the License. > <!-- ================================================================== --> > > <target name="start" > - description="Start OFBiz"> > + description="Start OFBiz (use -Dportoffset=portNumber to shift all ports with the portNumber value)"> > <java jar="ofbiz.jar" fork="true"> > <jvmarg value="${memory.initial.param}"/> > <jvmarg value="${memory.max.param}"/> > <jvmarg value="${memory.maxpermsize.param}"/> > + <arg value="start"/> > + <arg value="-portoffset=${portoffset}"/> > </java> > </target> > <target name="start-batch" > @@ -436,21 +438,23 @@ under the License. > </java> > </target> > <target name="start-pos" > - description="Start OFBiz POS (Point of sale)"> > + description="Start OFBiz POS (Point of sale). Use -Dportoffset=portNumber to shift all ports with the portNumber value."> > <java jar="ofbiz.jar" fork="true"> > <jvmarg value="${memory.initial.param}"/> > <jvmarg value="${memory.max.param}"/> > <jvmarg value="${memory.maxpermsize.param}"/> > <arg value="pos"/> > + <arg value="-portoffset=${portoffset}"/> > </java> > </target> > <target name="start-both" > - description="Start OFBiz in both Web and POS (Point of sale) modes"> > + description="Start OFBiz in both Web and POS (Point of sale) modes. Use -Dportoffset=portNumber to shift all ports with the portNumber value."> > <java jar="ofbiz.jar" fork="true"> > <jvmarg value="${memory.initial.param}"/> > <jvmarg value="${memory.max.param}"/> > <jvmarg value="${memory.maxpermsize.param}"/> > <arg value="both"/> > + <arg value="-portoffset=${portoffset}"/> > </java> > </target> > <target name="stop" > @@ -1296,7 +1300,7 @@ under the License. > <!-- kek helper --> > <!-- ================================================================== --> > > - <target name="gen-kek" > + <target name="gen-kek" > description="Generate a new key-encrypting-key for use in entityengine.xml"> > <java classname="org.ofbiz.base.crypto.Main" fork="false"> > <arg value="-kek"/> > > Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/container/ClassLoaderContainer.java > URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/container/ClassLoaderContainer.java?rev=1533839&r1=1533838&r2=1533839&view=diff > ============================================================================== > --- ofbiz/trunk/framework/base/src/org/ofbiz/base/container/ClassLoaderContainer.java (original) > +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/container/ClassLoaderContainer.java Sat Oct 19 22:07:35 2013 > @@ -18,11 +18,11 @@ > *******************************************************************************/ > package org.ofbiz.base.container; > > +import java.net.URL; > + > +import org.ofbiz.base.start.Classpath; > import org.ofbiz.base.util.CachedClassLoader; > import org.ofbiz.base.util.Debug; > -import org.ofbiz.base.start.Classpath; > - > -import java.net.URL; > > /** > * ClassLoader Container; Created a CachedClassLoader for use by all following containers > @@ -32,6 +32,7 @@ public class ClassLoaderContainer implem > > public static final String module = ClassLoaderContainer.class.getName(); > protected static CachedClassLoader cl = null; > + public static Integer portOffset = 0; > private String name; > > @Override > @@ -46,6 +47,34 @@ public class ClassLoaderContainer implem > } > > cl = new CachedClassLoader(new URL[0], parent); > + > + if (args != null) { > + for (String argument : args) { > + // arguments can prefix w/ a '-'. Just strip them off > + if (argument.startsWith("-")) { > + int subIdx = 1; > + if (argument.startsWith("--")) { > + subIdx = 2; > + } > + argument = argument.substring(subIdx); > + } > + > + // parse the arguments > + if (argument.indexOf("=") != -1) { > + String argumentName = argument.substring(0, argument.indexOf("=")); > + String argumentVal = argument.substring(argument.indexOf("=") + 1); > + > + if ("portoffset".equalsIgnoreCase(argumentName) && !"${portoffset}".equals(argumentVal)) { > + try { > + ClassLoaderContainer.portOffset = Integer.valueOf(argumentVal); > + } catch (NumberFormatException e) { > + e.printStackTrace(); > + } > + } > + } > + } > + } > + > Thread.currentThread().setContextClassLoader(cl); > Debug.logInfo("CachedClassLoader created", module); > } > > Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/container/NamingServiceContainer.java > URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/container/NamingServiceContainer.java?rev=1533839&r1=1533838&r2=1533839&view=diff > ============================================================================== > --- ofbiz/trunk/framework/base/src/org/ofbiz/base/container/NamingServiceContainer.java (original) > +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/container/NamingServiceContainer.java Sat Oct 19 22:07:35 2013 > @@ -57,9 +57,9 @@ public class NamingServiceContainer impl > ContainerConfig.Container.Property port = cfg.getProperty("port"); > if (port.value != null) { > try { > - this.namingPort = Integer.parseInt(port.value); > + this.namingPort = Integer.parseInt(port.value) + ClassLoaderContainer.portOffset; > } catch (Exception e) { > - throw new ContainerException("Invalid port defined in container [naming-container] configuration; not a valid int"); > + throw new ContainerException("Invalid port defined in container [naming-container] configuration or as portOffset; not a valid int"); > } > } > > > Modified: ofbiz/trunk/framework/catalina/src/org/ofbiz/catalina/container/CatalinaContainer.java > URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/catalina/src/org/ofbiz/catalina/container/CatalinaContainer.java?rev=1533839&r1=1533838&r2=1533839&view=diff > ============================================================================== > --- ofbiz/trunk/framework/catalina/src/org/ofbiz/catalina/container/CatalinaContainer.java (original) > +++ ofbiz/trunk/framework/catalina/src/org/ofbiz/catalina/container/CatalinaContainer.java Sat Oct 19 22:07:35 2013 > @@ -246,6 +246,8 @@ public class CatalinaContainer implement > > for (Connector con: tomcat.getService().findConnectors()) { > ProtocolHandler ph = con.getProtocolHandler(); > + int port = con.getPort(); > + con.setAttribute("port", port); > if (ph instanceof Http11Protocol) { > Http11Protocol hph = (Http11Protocol) ph; > Debug.logInfo("Connector " + hph.getName() + " @ " + hph.getPort() + " - " + > @@ -477,7 +479,8 @@ public class CatalinaContainer implement > // need some standard properties > String protocol = ContainerConfig.getPropertyValue(connectorProp, "protocol", "HTTP/1.1"); > String address = ContainerConfig.getPropertyValue(connectorProp, "address", "0.0.0.0"); > - int port = ContainerConfig.getPropertyValue(connectorProp, "port", 0); > + int port = ContainerConfig.getPropertyValue(connectorProp, "port", 0) + ClassLoaderContainer.portOffset; > + > boolean secure = ContainerConfig.getPropertyValue(connectorProp, "secure", false); > if (protocol.toLowerCase().startsWith("ajp")) { > protocol = "ajp"; > @@ -537,8 +540,12 @@ public class CatalinaContainer implement > > try { > for (ContainerConfig.Container.Property prop: connectorProp.properties.values()) { > - connector.setProperty(prop.name, prop.value); > - //connector.setAttribute(prop.name, prop.value); > + if ("port".equals(prop.name)) { > + connector.setProperty(prop.name, "" + port); > + } else { > + connector.setProperty(prop.name, prop.value); > + //connector.setAttribute(prop.name, prop.value); > + } > } > > if (connectorProp.properties.containsKey("URIEncoding")) { > > Modified: ofbiz/trunk/framework/common/servicedef/services_test.xml > URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/common/servicedef/services_test.xml?rev=1533839&r1=1533838&r2=1533839&view=diff > ============================================================================== > --- ofbiz/trunk/framework/common/servicedef/services_test.xml (original) > +++ ofbiz/trunk/framework/common/servicedef/services_test.xml Sat Oct 19 22:07:35 2013 > @@ -72,21 +72,18 @@ under the License. > > <service name="groupTest" engine="group" location="testGroup" invoke=""/> > > - <service name="testHttp" engine="http" > - location="http://localhost:8080/webtools/control/httpService" invoke="testScv"> > + <service name="testHttp" engine="http" location="main-http" invoke="testScv"> > <description>HTTP service wrapper around the test service</description> > <attribute name="message" type="String" mode="IN" optional="true"/> > <attribute name="resp" type="String" mode="OUT"/> > </service> > > - <service name="testSoap" engine="soap" export="true" > - location="http://localhost:8080/webtools/control/SOAPService" invoke="testSOAPScv"> > + <service name="testSoap" engine="soap" export="true" location="main-soap" invoke="testSOAPScv"> > <description>SOAP service; calls the OFBiz test SOAP service</description> > <implements service="testSOAPScv"/> > </service> > > - <service name="testSoapSimple" engine="soap" export="true" > - location="http://localhost:8080/webtools/control/SOAPService" invoke="testScv"> > + <service name="testSoapSimple" engine="soap" export="true" location="main-soap" invoke="testScv"> > <description>simple SOAP service; calls the OFBiz test service</description> > <implements service="testScv"/> > </service> > > Modified: ofbiz/trunk/framework/service/config/serviceengine.xml > URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/service/config/serviceengine.xml?rev=1533839&r1=1533838&r2=1533839&view=diff > ============================================================================== > --- ofbiz/trunk/framework/service/config/serviceengine.xml (original) > +++ ofbiz/trunk/framework/service/config/serviceengine.xml Sat Oct 19 22:07:35 2013 > @@ -59,9 +59,8 @@ under the License. > <engine name="jms" class="org.ofbiz.service.jms.JmsServiceEngine"/> > <engine name="rmi" class="org.ofbiz.service.rmi.RmiServiceEngine"/> > <engine name="soap" class="org.ofbiz.service.engine.SOAPClientEngine"/> > - <!-- The engine xml-rpc-local is only used by a test service and for > - this reason it is configured to run on port 8080 (see rmi-dispatcher in service/ofbiz-component.xml); > - in order to use this in OFBiz change the port accordingly (for demo the default value is 8080) > + <!-- The engine xml-rpc-local is only used by a test service and for this reason it is configured to run on port 8080. > + In order to use this in OFBiz change the port accordingly (for demo the default value is 8080) > --> > <engine name="xml-rpc-local" class="org.ofbiz.service.engine.XMLRPCClientEngine"> > <parameter name="url" value="http://localhost:8080/webtools/control/xmlrpc"/> > @@ -71,7 +70,8 @@ under the License. > > <service-location name="main-rmi" location="rmi://localhost:1099/RMIDispatcher"/> > <service-location name="main-http" location="http://localhost:8080/webtools/control/httpService"/> > - > + <service-location name="main-soap" location="http://localhost:8080/webtools/control/SOAPService"/> > + > <service-location name="entity-sync-rmi" location="rmi://localhost:1099/RMIDispatcher"/> > <service-location name="entity-sync-http" location="http://localhost:8080/webtools/control/httpService"/> > > > Modified: ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceEngine.java > URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceEngine.java?rev=1533839&r1=1533838&r2=1533839&view=diff > ============================================================================== > --- ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceEngine.java (original) > +++ ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceEngine.java Sat Oct 19 22:07:35 2013 > @@ -24,6 +24,7 @@ import java.util.HashMap; > import java.util.List; > import java.util.Map; > > +import org.ofbiz.base.container.ClassLoaderContainer; > import org.ofbiz.base.lang.ThreadSafe; > import org.ofbiz.base.util.UtilXml; > import org.ofbiz.service.config.ServiceConfigException; > @@ -88,6 +89,16 @@ public final class ServiceEngine { > for (Element serviceLocationElement : serviceLocationElementList) { > serviceLocations.add(new ServiceLocation(serviceLocationElement)); > } > + for (ServiceLocation serviceLocation : serviceLocations) { > + String location = serviceLocation.getLocation(); > + if (location.contains("localhost") && ClassLoaderContainer.portOffset != 0) { > + Integer port = 1099 + ClassLoaderContainer.portOffset; > + location = location.replace("1099", port.toString()); > + port = 8080 + ClassLoaderContainer.portOffset; > + location = location.replace("8080", port.toString()); > + serviceLocation.setLocation(location); > + } > + } > this.serviceLocations = Collections.unmodifiableList(serviceLocations); > } > List<? extends Element> notificationGroupElementList = UtilXml.childElementList(engineElement, "notification-group"); > > Modified: ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceLocation.java > URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceLocation.java?rev=1533839&r1=1533838&r2=1533839&view=diff > ============================================================================== > --- ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceLocation.java (original) > +++ ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceLocation.java Sat Oct 19 22:07:35 2013 > @@ -28,7 +28,7 @@ import org.w3c.dom.Element; > @ThreadSafe > public final class ServiceLocation { > > - private final String location; > + private String location; > private final String name; > > ServiceLocation(Element serviceLocationElement) throws ServiceConfigException { > @@ -48,6 +48,10 @@ public final class ServiceLocation { > return location; > } > > + public void setLocation(String location) { > + this.location = location; > + } > + > public String getName() { > return name; > } > > Modified: ofbiz/trunk/framework/service/src/org/ofbiz/service/engine/XMLRPCClientEngine.java > URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/service/src/org/ofbiz/service/engine/XMLRPCClientEngine.java?rev=1533839&r1=1533838&r2=1533839&view=diff > ============================================================================== > --- ofbiz/trunk/framework/service/src/org/ofbiz/service/engine/XMLRPCClientEngine.java (original) > +++ ofbiz/trunk/framework/service/src/org/ofbiz/service/engine/XMLRPCClientEngine.java Sat Oct 19 22:07:35 2013 > @@ -28,6 +28,7 @@ import javolution.util.FastMap; > import org.apache.xmlrpc.XmlRpcException; > import org.apache.xmlrpc.client.XmlRpcClientConfigImpl; > import org.ofbiz.base.config.GenericConfigException; > +import org.ofbiz.base.container.ClassLoaderContainer; > import org.ofbiz.base.util.Debug; > import org.ofbiz.base.util.UtilGenerics; > import org.ofbiz.base.util.UtilMisc; > @@ -90,6 +91,10 @@ public class XMLRPCClientEngine extends > String keyAlias = null; > try { > url = ServiceConfigUtil.getEngineParameter(engine, "url"); > + if (ClassLoaderContainer.portOffset != 0) { > + Integer port = 8080 + ClassLoaderContainer.portOffset; > + url = url.replace("8080", port.toString()); > + } > login = ServiceConfigUtil.getEngineParameter(engine, "login"); > password = ServiceConfigUtil.getEngineParameter(engine, "password"); > keyStoreComponent = ServiceConfigUtil.getEngineParameter(engine, "keyStoreComponent"); > > Modified: ofbiz/trunk/framework/service/src/org/ofbiz/service/rmi/RmiServiceContainer.java > URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/service/src/org/ofbiz/service/rmi/RmiServiceContainer.java?rev=1533839&r1=1533838&r2=1533839&view=diff > ============================================================================== > --- ofbiz/trunk/framework/service/src/org/ofbiz/service/rmi/RmiServiceContainer.java (original) > +++ ofbiz/trunk/framework/service/src/org/ofbiz/service/rmi/RmiServiceContainer.java Sat Oct 19 22:07:35 2013 > @@ -26,6 +26,7 @@ import java.rmi.server.RMIServerSocketFa > import javax.naming.InitialContext; > import javax.naming.NamingException; > > +import org.ofbiz.base.container.ClassLoaderContainer; > import org.ofbiz.base.container.Container; > import org.ofbiz.base.container.ContainerConfig; > import org.ofbiz.base.container.ContainerException; > @@ -80,6 +81,11 @@ public class RmiServiceContainer impleme > String useCtx = initialCtxProp == null || initialCtxProp.value == null ? "false" : initialCtxProp.value; > String host = lookupHostProp == null || lookupHostProp.value == null ? "localhost" : lookupHostProp.value; > String port = lookupPortProp == null || lookupPortProp.value == null ? "1099" : lookupPortProp.value; > + if (ClassLoaderContainer.portOffset != 0) { > + Integer portValue = Integer.valueOf(port); > + portValue += ClassLoaderContainer.portOffset; > + port = portValue.toString(); > + } > String keystore = ContainerConfig.getPropertyValue(cfg, "ssl-keystore", null); > String ksType = ContainerConfig.getPropertyValue(cfg, "ssl-keystore-type", "JKS"); > String ksPass = ContainerConfig.getPropertyValue(cfg, "ssl-keystore-pass", null); > > Modified: ofbiz/trunk/framework/service/src/org/ofbiz/service/test/XmlRpcTests.java > URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/service/src/org/ofbiz/service/test/XmlRpcTests.java?rev=1533839&r1=1533838&r2=1533839&view=diff > ============================================================================== > --- ofbiz/trunk/framework/service/src/org/ofbiz/service/test/XmlRpcTests.java (original) > +++ ofbiz/trunk/framework/service/src/org/ofbiz/service/test/XmlRpcTests.java Sat Oct 19 22:07:35 2013 > @@ -23,6 +23,7 @@ import java.util.Locale; > import java.util.Map; > > import org.apache.xmlrpc.client.XmlRpcClient; > +import org.ofbiz.base.container.ClassLoaderContainer; > import org.ofbiz.base.util.UtilGenerics; > import org.ofbiz.base.util.UtilProperties; > import org.ofbiz.base.util.UtilValidate; > @@ -37,10 +38,14 @@ public class XmlRpcTests extends Abstrac > > public static final String module = XmlRpcTests.class.getName(); > public static final String resource = "ServiceErrorUiLabels"; > - public static final String url = "http://localhost:8080/webtools/control/xmlrpc"; > + public static String url = "http://localhost:8080/webtools/control/xmlrpc"; > > public XmlRpcTests(String name) { > super(name); > + if (ClassLoaderContainer.portOffset != 0) { > + Integer port = 8080 + ClassLoaderContainer.portOffset; > + url = url.replace("8080", port.toString()); > + } > } > > /** > > Modified: ofbiz/trunk/framework/start/src/org/ofbiz/base/start/Config.java > URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/start/src/org/ofbiz/base/start/Config.java?rev=1533839&r1=1533838&r2=1533839&view=diff > ============================================================================== > --- ofbiz/trunk/framework/start/src/org/ofbiz/base/start/Config.java (original) > +++ ofbiz/trunk/framework/start/src/org/ofbiz/base/start/Config.java Sat Oct 19 22:07:35 2013 > @@ -57,7 +57,7 @@ public class Config { > String firstArg = args.length > 0 ? args[0] : ""; > String configFileName = getConfigFileName(firstArg); > Config result = new Config(); > - result.readConfig(configFileName); > + result.readConfig(configFileName, args); > return result; > } > > @@ -283,7 +283,7 @@ public class Config { > } > } > > - public void readConfig(String config) throws IOException { > + public void readConfig(String config, String[] args) throws IOException { > // check the java_version > String javaVersion = System.getProperty("java.version"); > String javaVendor = System.getProperty("java.vendor"); > @@ -352,6 +352,14 @@ public class Config { > // parse the port number > try { > adminPort = Integer.parseInt(adminPortStr); > + if (args.length > 0) { > + for (String arg : args) { > + if (arg.toLowerCase().contains("portoffset=")) { > + adminPort = adminPort != 0 ? adminPort : 10523; // This is necessary because the ASF machines don't allow ports 1 to 3, see INFRA-6790 > + adminPort += Integer.parseInt(arg.split("=")[1]); > + } > + } > + } > } catch (Exception e) { > adminPort = 0; > } > @@ -396,16 +404,16 @@ public class Config { > // set the default locale > String localeString = props.getProperty("ofbiz.locale.default"); > if (localeString != null && localeString.length() > 0) { > - String args[] = localeString.split("_"); > - switch (args.length) { > + String locales[] = localeString.split("_"); > + switch (locales.length) { > case 1: > - Locale.setDefault(new Locale(args[0])); > + Locale.setDefault(new Locale(locales[0])); > break; > case 2: > - Locale.setDefault(new Locale(args[0], args[1])); > + Locale.setDefault(new Locale(locales[0], locales[1])); > break; > case 3: > - Locale.setDefault(new Locale(args[0], args[1], args[2])); > + Locale.setDefault(new Locale(locales[0], locales[1], args[2])); > } > System.setProperty("user.language", localeString); > } > > |
Re: svn commit: r1533839 - in /ofbiz/trunk: ./ framework/base/src/org/ofbiz/b
Administrator
|
It was the easiest way I found to pass it everywhere I needed to change the port
Jacques Le 12/09/2014 17:20, Jacopo Cappellato a écrit : > Jacques, > > what was the rationale behind your design decision to add the portOffset field to the ClassLoaderContainer? Why ClassLoaderContainer? It doesn't make much sense to me... > > Jacopo > > On Oct 20, 2013, at 12:07 AM, [hidden email] wrote: > >> Author: jleroux >> Date: Sat Oct 19 22:07:35 2013 >> New Revision: 1533839 >> >> URL: http://svn.apache.org/r1533839 >> Log: >> Set different ports for testing in a CI environment (e.g. Jenkins) https://issues.apache.org/jira/browse/OFBIZ-4794 >> >> *What's for?* >> Finally, OOTB the port offset is interesting in 2 cases: >> # To eventually run Sonar on OFBiz sources https://issues.apache.org/jira/browse/INFRA-3590 >> # To simultaneously run the demos without having to change much things, just one parameter (this will need to have the same commited in R13.07... to be discussed...) >> >> Of course, It can be used for other purposes outside of the ASF context... >> >> *How it's done?* >> Basically, I just added a portoffset integer parameter to 3 ant targets (of course this JVM param can also be used in a Java call from command line): start, start-pos, start-both, >> >> The portoffset parameter is set to zero by default in ClassLoaderContainer.java. >> If a value is passed, it's then grabed by Config.java and passed to ClassLoaderContainer.java, where it's set to this value. >> >> The portoffset is then used to change in one shoot all the default ports values defined (in config or hardcoded), and the admin port value >> This is done in following classes: >> CatalinaContainer >> NamingServiceContainer >> ServiceEngine through ServiceLocation >> RmiServiceContainer >> XMLRPCClientEngine >> XmlRpcTests >> >> To avoid hardcoding locations in services definitions, I also created 2 service-locations: main-http and main-soap >> >> *To summarize:* these changes will be used to run "ant start" with a portoffset value which will take care of all ports values, including admin port. For running R12.04 demo as stable we will still need to create a patch for the last time. R13.07 could have a similar patch applied since it's not yet released... >> >> Modified: >> ofbiz/trunk/build.xml >> ofbiz/trunk/framework/base/src/org/ofbiz/base/container/ClassLoaderContainer.java >> ofbiz/trunk/framework/base/src/org/ofbiz/base/container/NamingServiceContainer.java >> ofbiz/trunk/framework/catalina/src/org/ofbiz/catalina/container/CatalinaContainer.java >> ofbiz/trunk/framework/common/servicedef/services_test.xml >> ofbiz/trunk/framework/service/config/serviceengine.xml >> ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceEngine.java >> ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceLocation.java >> ofbiz/trunk/framework/service/src/org/ofbiz/service/engine/XMLRPCClientEngine.java >> ofbiz/trunk/framework/service/src/org/ofbiz/service/rmi/RmiServiceContainer.java >> ofbiz/trunk/framework/service/src/org/ofbiz/service/test/XmlRpcTests.java >> ofbiz/trunk/framework/start/src/org/ofbiz/base/start/Config.java >> >> Modified: ofbiz/trunk/build.xml >> URL: http://svn.apache.org/viewvc/ofbiz/trunk/build.xml?rev=1533839&r1=1533838&r2=1533839&view=diff >> ============================================================================== >> --- ofbiz/trunk/build.xml (original) >> +++ ofbiz/trunk/build.xml Sat Oct 19 22:07:35 2013 >> @@ -408,11 +408,13 @@ under the License. >> <!-- ================================================================== --> >> >> <target name="start" >> - description="Start OFBiz"> >> + description="Start OFBiz (use -Dportoffset=portNumber to shift all ports with the portNumber value)"> >> <java jar="ofbiz.jar" fork="true"> >> <jvmarg value="${memory.initial.param}"/> >> <jvmarg value="${memory.max.param}"/> >> <jvmarg value="${memory.maxpermsize.param}"/> >> + <arg value="start"/> >> + <arg value="-portoffset=${portoffset}"/> >> </java> >> </target> >> <target name="start-batch" >> @@ -436,21 +438,23 @@ under the License. >> </java> >> </target> >> <target name="start-pos" >> - description="Start OFBiz POS (Point of sale)"> >> + description="Start OFBiz POS (Point of sale). Use -Dportoffset=portNumber to shift all ports with the portNumber value."> >> <java jar="ofbiz.jar" fork="true"> >> <jvmarg value="${memory.initial.param}"/> >> <jvmarg value="${memory.max.param}"/> >> <jvmarg value="${memory.maxpermsize.param}"/> >> <arg value="pos"/> >> + <arg value="-portoffset=${portoffset}"/> >> </java> >> </target> >> <target name="start-both" >> - description="Start OFBiz in both Web and POS (Point of sale) modes"> >> + description="Start OFBiz in both Web and POS (Point of sale) modes. Use -Dportoffset=portNumber to shift all ports with the portNumber value."> >> <java jar="ofbiz.jar" fork="true"> >> <jvmarg value="${memory.initial.param}"/> >> <jvmarg value="${memory.max.param}"/> >> <jvmarg value="${memory.maxpermsize.param}"/> >> <arg value="both"/> >> + <arg value="-portoffset=${portoffset}"/> >> </java> >> </target> >> <target name="stop" >> @@ -1296,7 +1300,7 @@ under the License. >> <!-- kek helper --> >> <!-- ================================================================== --> >> >> - <target name="gen-kek" >> + <target name="gen-kek" >> description="Generate a new key-encrypting-key for use in entityengine.xml"> >> <java classname="org.ofbiz.base.crypto.Main" fork="false"> >> <arg value="-kek"/> >> >> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/container/ClassLoaderContainer.java >> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/container/ClassLoaderContainer.java?rev=1533839&r1=1533838&r2=1533839&view=diff >> ============================================================================== >> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/container/ClassLoaderContainer.java (original) >> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/container/ClassLoaderContainer.java Sat Oct 19 22:07:35 2013 >> @@ -18,11 +18,11 @@ >> *******************************************************************************/ >> package org.ofbiz.base.container; >> >> +import java.net.URL; >> + >> +import org.ofbiz.base.start.Classpath; >> import org.ofbiz.base.util.CachedClassLoader; >> import org.ofbiz.base.util.Debug; >> -import org.ofbiz.base.start.Classpath; >> - >> -import java.net.URL; >> >> /** >> * ClassLoader Container; Created a CachedClassLoader for use by all following containers >> @@ -32,6 +32,7 @@ public class ClassLoaderContainer implem >> >> public static final String module = ClassLoaderContainer.class.getName(); >> protected static CachedClassLoader cl = null; >> + public static Integer portOffset = 0; >> private String name; >> >> @Override >> @@ -46,6 +47,34 @@ public class ClassLoaderContainer implem >> } >> >> cl = new CachedClassLoader(new URL[0], parent); >> + >> + if (args != null) { >> + for (String argument : args) { >> + // arguments can prefix w/ a '-'. Just strip them off >> + if (argument.startsWith("-")) { >> + int subIdx = 1; >> + if (argument.startsWith("--")) { >> + subIdx = 2; >> + } >> + argument = argument.substring(subIdx); >> + } >> + >> + // parse the arguments >> + if (argument.indexOf("=") != -1) { >> + String argumentName = argument.substring(0, argument.indexOf("=")); >> + String argumentVal = argument.substring(argument.indexOf("=") + 1); >> + >> + if ("portoffset".equalsIgnoreCase(argumentName) && !"${portoffset}".equals(argumentVal)) { >> + try { >> + ClassLoaderContainer.portOffset = Integer.valueOf(argumentVal); >> + } catch (NumberFormatException e) { >> + e.printStackTrace(); >> + } >> + } >> + } >> + } >> + } >> + >> Thread.currentThread().setContextClassLoader(cl); >> Debug.logInfo("CachedClassLoader created", module); >> } >> >> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/container/NamingServiceContainer.java >> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/container/NamingServiceContainer.java?rev=1533839&r1=1533838&r2=1533839&view=diff >> ============================================================================== >> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/container/NamingServiceContainer.java (original) >> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/container/NamingServiceContainer.java Sat Oct 19 22:07:35 2013 >> @@ -57,9 +57,9 @@ public class NamingServiceContainer impl >> ContainerConfig.Container.Property port = cfg.getProperty("port"); >> if (port.value != null) { >> try { >> - this.namingPort = Integer.parseInt(port.value); >> + this.namingPort = Integer.parseInt(port.value) + ClassLoaderContainer.portOffset; >> } catch (Exception e) { >> - throw new ContainerException("Invalid port defined in container [naming-container] configuration; not a valid int"); >> + throw new ContainerException("Invalid port defined in container [naming-container] configuration or as portOffset; not a valid int"); >> } >> } >> >> >> Modified: ofbiz/trunk/framework/catalina/src/org/ofbiz/catalina/container/CatalinaContainer.java >> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/catalina/src/org/ofbiz/catalina/container/CatalinaContainer.java?rev=1533839&r1=1533838&r2=1533839&view=diff >> ============================================================================== >> --- ofbiz/trunk/framework/catalina/src/org/ofbiz/catalina/container/CatalinaContainer.java (original) >> +++ ofbiz/trunk/framework/catalina/src/org/ofbiz/catalina/container/CatalinaContainer.java Sat Oct 19 22:07:35 2013 >> @@ -246,6 +246,8 @@ public class CatalinaContainer implement >> >> for (Connector con: tomcat.getService().findConnectors()) { >> ProtocolHandler ph = con.getProtocolHandler(); >> + int port = con.getPort(); >> + con.setAttribute("port", port); >> if (ph instanceof Http11Protocol) { >> Http11Protocol hph = (Http11Protocol) ph; >> Debug.logInfo("Connector " + hph.getName() + " @ " + hph.getPort() + " - " + >> @@ -477,7 +479,8 @@ public class CatalinaContainer implement >> // need some standard properties >> String protocol = ContainerConfig.getPropertyValue(connectorProp, "protocol", "HTTP/1.1"); >> String address = ContainerConfig.getPropertyValue(connectorProp, "address", "0.0.0.0"); >> - int port = ContainerConfig.getPropertyValue(connectorProp, "port", 0); >> + int port = ContainerConfig.getPropertyValue(connectorProp, "port", 0) + ClassLoaderContainer.portOffset; >> + >> boolean secure = ContainerConfig.getPropertyValue(connectorProp, "secure", false); >> if (protocol.toLowerCase().startsWith("ajp")) { >> protocol = "ajp"; >> @@ -537,8 +540,12 @@ public class CatalinaContainer implement >> >> try { >> for (ContainerConfig.Container.Property prop: connectorProp.properties.values()) { >> - connector.setProperty(prop.name, prop.value); >> - //connector.setAttribute(prop.name, prop.value); >> + if ("port".equals(prop.name)) { >> + connector.setProperty(prop.name, "" + port); >> + } else { >> + connector.setProperty(prop.name, prop.value); >> + //connector.setAttribute(prop.name, prop.value); >> + } >> } >> >> if (connectorProp.properties.containsKey("URIEncoding")) { >> >> Modified: ofbiz/trunk/framework/common/servicedef/services_test.xml >> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/common/servicedef/services_test.xml?rev=1533839&r1=1533838&r2=1533839&view=diff >> ============================================================================== >> --- ofbiz/trunk/framework/common/servicedef/services_test.xml (original) >> +++ ofbiz/trunk/framework/common/servicedef/services_test.xml Sat Oct 19 22:07:35 2013 >> @@ -72,21 +72,18 @@ under the License. >> >> <service name="groupTest" engine="group" location="testGroup" invoke=""/> >> >> - <service name="testHttp" engine="http" >> - location="http://localhost:8080/webtools/control/httpService" invoke="testScv"> >> + <service name="testHttp" engine="http" location="main-http" invoke="testScv"> >> <description>HTTP service wrapper around the test service</description> >> <attribute name="message" type="String" mode="IN" optional="true"/> >> <attribute name="resp" type="String" mode="OUT"/> >> </service> >> >> - <service name="testSoap" engine="soap" export="true" >> - location="http://localhost:8080/webtools/control/SOAPService" invoke="testSOAPScv"> >> + <service name="testSoap" engine="soap" export="true" location="main-soap" invoke="testSOAPScv"> >> <description>SOAP service; calls the OFBiz test SOAP service</description> >> <implements service="testSOAPScv"/> >> </service> >> >> - <service name="testSoapSimple" engine="soap" export="true" >> - location="http://localhost:8080/webtools/control/SOAPService" invoke="testScv"> >> + <service name="testSoapSimple" engine="soap" export="true" location="main-soap" invoke="testScv"> >> <description>simple SOAP service; calls the OFBiz test service</description> >> <implements service="testScv"/> >> </service> >> >> Modified: ofbiz/trunk/framework/service/config/serviceengine.xml >> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/service/config/serviceengine.xml?rev=1533839&r1=1533838&r2=1533839&view=diff >> ============================================================================== >> --- ofbiz/trunk/framework/service/config/serviceengine.xml (original) >> +++ ofbiz/trunk/framework/service/config/serviceengine.xml Sat Oct 19 22:07:35 2013 >> @@ -59,9 +59,8 @@ under the License. >> <engine name="jms" class="org.ofbiz.service.jms.JmsServiceEngine"/> >> <engine name="rmi" class="org.ofbiz.service.rmi.RmiServiceEngine"/> >> <engine name="soap" class="org.ofbiz.service.engine.SOAPClientEngine"/> >> - <!-- The engine xml-rpc-local is only used by a test service and for >> - this reason it is configured to run on port 8080 (see rmi-dispatcher in service/ofbiz-component.xml); >> - in order to use this in OFBiz change the port accordingly (for demo the default value is 8080) >> + <!-- The engine xml-rpc-local is only used by a test service and for this reason it is configured to run on port 8080. >> + In order to use this in OFBiz change the port accordingly (for demo the default value is 8080) >> --> >> <engine name="xml-rpc-local" class="org.ofbiz.service.engine.XMLRPCClientEngine"> >> <parameter name="url" value="http://localhost:8080/webtools/control/xmlrpc"/> >> @@ -71,7 +70,8 @@ under the License. >> >> <service-location name="main-rmi" location="rmi://localhost:1099/RMIDispatcher"/> >> <service-location name="main-http" location="http://localhost:8080/webtools/control/httpService"/> >> - >> + <service-location name="main-soap" location="http://localhost:8080/webtools/control/SOAPService"/> >> + >> <service-location name="entity-sync-rmi" location="rmi://localhost:1099/RMIDispatcher"/> >> <service-location name="entity-sync-http" location="http://localhost:8080/webtools/control/httpService"/> >> >> >> Modified: ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceEngine.java >> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceEngine.java?rev=1533839&r1=1533838&r2=1533839&view=diff >> ============================================================================== >> --- ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceEngine.java (original) >> +++ ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceEngine.java Sat Oct 19 22:07:35 2013 >> @@ -24,6 +24,7 @@ import java.util.HashMap; >> import java.util.List; >> import java.util.Map; >> >> +import org.ofbiz.base.container.ClassLoaderContainer; >> import org.ofbiz.base.lang.ThreadSafe; >> import org.ofbiz.base.util.UtilXml; >> import org.ofbiz.service.config.ServiceConfigException; >> @@ -88,6 +89,16 @@ public final class ServiceEngine { >> for (Element serviceLocationElement : serviceLocationElementList) { >> serviceLocations.add(new ServiceLocation(serviceLocationElement)); >> } >> + for (ServiceLocation serviceLocation : serviceLocations) { >> + String location = serviceLocation.getLocation(); >> + if (location.contains("localhost") && ClassLoaderContainer.portOffset != 0) { >> + Integer port = 1099 + ClassLoaderContainer.portOffset; >> + location = location.replace("1099", port.toString()); >> + port = 8080 + ClassLoaderContainer.portOffset; >> + location = location.replace("8080", port.toString()); >> + serviceLocation.setLocation(location); >> + } >> + } >> this.serviceLocations = Collections.unmodifiableList(serviceLocations); >> } >> List<? extends Element> notificationGroupElementList = UtilXml.childElementList(engineElement, "notification-group"); >> >> Modified: ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceLocation.java >> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceLocation.java?rev=1533839&r1=1533838&r2=1533839&view=diff >> ============================================================================== >> --- ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceLocation.java (original) >> +++ ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceLocation.java Sat Oct 19 22:07:35 2013 >> @@ -28,7 +28,7 @@ import org.w3c.dom.Element; >> @ThreadSafe >> public final class ServiceLocation { >> >> - private final String location; >> + private String location; >> private final String name; >> >> ServiceLocation(Element serviceLocationElement) throws ServiceConfigException { >> @@ -48,6 +48,10 @@ public final class ServiceLocation { >> return location; >> } >> >> + public void setLocation(String location) { >> + this.location = location; >> + } >> + >> public String getName() { >> return name; >> } >> >> Modified: ofbiz/trunk/framework/service/src/org/ofbiz/service/engine/XMLRPCClientEngine.java >> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/service/src/org/ofbiz/service/engine/XMLRPCClientEngine.java?rev=1533839&r1=1533838&r2=1533839&view=diff >> ============================================================================== >> --- ofbiz/trunk/framework/service/src/org/ofbiz/service/engine/XMLRPCClientEngine.java (original) >> +++ ofbiz/trunk/framework/service/src/org/ofbiz/service/engine/XMLRPCClientEngine.java Sat Oct 19 22:07:35 2013 >> @@ -28,6 +28,7 @@ import javolution.util.FastMap; >> import org.apache.xmlrpc.XmlRpcException; >> import org.apache.xmlrpc.client.XmlRpcClientConfigImpl; >> import org.ofbiz.base.config.GenericConfigException; >> +import org.ofbiz.base.container.ClassLoaderContainer; >> import org.ofbiz.base.util.Debug; >> import org.ofbiz.base.util.UtilGenerics; >> import org.ofbiz.base.util.UtilMisc; >> @@ -90,6 +91,10 @@ public class XMLRPCClientEngine extends >> String keyAlias = null; >> try { >> url = ServiceConfigUtil.getEngineParameter(engine, "url"); >> + if (ClassLoaderContainer.portOffset != 0) { >> + Integer port = 8080 + ClassLoaderContainer.portOffset; >> + url = url.replace("8080", port.toString()); >> + } >> login = ServiceConfigUtil.getEngineParameter(engine, "login"); >> password = ServiceConfigUtil.getEngineParameter(engine, "password"); >> keyStoreComponent = ServiceConfigUtil.getEngineParameter(engine, "keyStoreComponent"); >> >> Modified: ofbiz/trunk/framework/service/src/org/ofbiz/service/rmi/RmiServiceContainer.java >> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/service/src/org/ofbiz/service/rmi/RmiServiceContainer.java?rev=1533839&r1=1533838&r2=1533839&view=diff >> ============================================================================== >> --- ofbiz/trunk/framework/service/src/org/ofbiz/service/rmi/RmiServiceContainer.java (original) >> +++ ofbiz/trunk/framework/service/src/org/ofbiz/service/rmi/RmiServiceContainer.java Sat Oct 19 22:07:35 2013 >> @@ -26,6 +26,7 @@ import java.rmi.server.RMIServerSocketFa >> import javax.naming.InitialContext; >> import javax.naming.NamingException; >> >> +import org.ofbiz.base.container.ClassLoaderContainer; >> import org.ofbiz.base.container.Container; >> import org.ofbiz.base.container.ContainerConfig; >> import org.ofbiz.base.container.ContainerException; >> @@ -80,6 +81,11 @@ public class RmiServiceContainer impleme >> String useCtx = initialCtxProp == null || initialCtxProp.value == null ? "false" : initialCtxProp.value; >> String host = lookupHostProp == null || lookupHostProp.value == null ? "localhost" : lookupHostProp.value; >> String port = lookupPortProp == null || lookupPortProp.value == null ? "1099" : lookupPortProp.value; >> + if (ClassLoaderContainer.portOffset != 0) { >> + Integer portValue = Integer.valueOf(port); >> + portValue += ClassLoaderContainer.portOffset; >> + port = portValue.toString(); >> + } >> String keystore = ContainerConfig.getPropertyValue(cfg, "ssl-keystore", null); >> String ksType = ContainerConfig.getPropertyValue(cfg, "ssl-keystore-type", "JKS"); >> String ksPass = ContainerConfig.getPropertyValue(cfg, "ssl-keystore-pass", null); >> >> Modified: ofbiz/trunk/framework/service/src/org/ofbiz/service/test/XmlRpcTests.java >> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/service/src/org/ofbiz/service/test/XmlRpcTests.java?rev=1533839&r1=1533838&r2=1533839&view=diff >> ============================================================================== >> --- ofbiz/trunk/framework/service/src/org/ofbiz/service/test/XmlRpcTests.java (original) >> +++ ofbiz/trunk/framework/service/src/org/ofbiz/service/test/XmlRpcTests.java Sat Oct 19 22:07:35 2013 >> @@ -23,6 +23,7 @@ import java.util.Locale; >> import java.util.Map; >> >> import org.apache.xmlrpc.client.XmlRpcClient; >> +import org.ofbiz.base.container.ClassLoaderContainer; >> import org.ofbiz.base.util.UtilGenerics; >> import org.ofbiz.base.util.UtilProperties; >> import org.ofbiz.base.util.UtilValidate; >> @@ -37,10 +38,14 @@ public class XmlRpcTests extends Abstrac >> >> public static final String module = XmlRpcTests.class.getName(); >> public static final String resource = "ServiceErrorUiLabels"; >> - public static final String url = "http://localhost:8080/webtools/control/xmlrpc"; >> + public static String url = "http://localhost:8080/webtools/control/xmlrpc"; >> >> public XmlRpcTests(String name) { >> super(name); >> + if (ClassLoaderContainer.portOffset != 0) { >> + Integer port = 8080 + ClassLoaderContainer.portOffset; >> + url = url.replace("8080", port.toString()); >> + } >> } >> >> /** >> >> Modified: ofbiz/trunk/framework/start/src/org/ofbiz/base/start/Config.java >> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/start/src/org/ofbiz/base/start/Config.java?rev=1533839&r1=1533838&r2=1533839&view=diff >> ============================================================================== >> --- ofbiz/trunk/framework/start/src/org/ofbiz/base/start/Config.java (original) >> +++ ofbiz/trunk/framework/start/src/org/ofbiz/base/start/Config.java Sat Oct 19 22:07:35 2013 >> @@ -57,7 +57,7 @@ public class Config { >> String firstArg = args.length > 0 ? args[0] : ""; >> String configFileName = getConfigFileName(firstArg); >> Config result = new Config(); >> - result.readConfig(configFileName); >> + result.readConfig(configFileName, args); >> return result; >> } >> >> @@ -283,7 +283,7 @@ public class Config { >> } >> } >> >> - public void readConfig(String config) throws IOException { >> + public void readConfig(String config, String[] args) throws IOException { >> // check the java_version >> String javaVersion = System.getProperty("java.version"); >> String javaVendor = System.getProperty("java.vendor"); >> @@ -352,6 +352,14 @@ public class Config { >> // parse the port number >> try { >> adminPort = Integer.parseInt(adminPortStr); >> + if (args.length > 0) { >> + for (String arg : args) { >> + if (arg.toLowerCase().contains("portoffset=")) { >> + adminPort = adminPort != 0 ? adminPort : 10523; // This is necessary because the ASF machines don't allow ports 1 to 3, see INFRA-6790 >> + adminPort += Integer.parseInt(arg.split("=")[1]); >> + } >> + } >> + } >> } catch (Exception e) { >> adminPort = 0; >> } >> @@ -396,16 +404,16 @@ public class Config { >> // set the default locale >> String localeString = props.getProperty("ofbiz.locale.default"); >> if (localeString != null && localeString.length() > 0) { >> - String args[] = localeString.split("_"); >> - switch (args.length) { >> + String locales[] = localeString.split("_"); >> + switch (locales.length) { >> case 1: >> - Locale.setDefault(new Locale(args[0])); >> + Locale.setDefault(new Locale(locales[0])); >> break; >> case 2: >> - Locale.setDefault(new Locale(args[0], args[1])); >> + Locale.setDefault(new Locale(locales[0], locales[1])); >> break; >> case 3: >> - Locale.setDefault(new Locale(args[0], args[1], args[2])); >> + Locale.setDefault(new Locale(locales[0], locales[1], args[2])); >> } >> System.setProperty("user.language", localeString); >> } >> >> > > |
Re: svn commit: r1533839 - in /ofbiz/trunk: ./ framework/base/src/org/ofbiz/b
|
|
In reply to this post by Jacopo Cappellato-3
With this change you have broken the thread safety of the ServiceLocation.java class; where you aware of this?
Jacopo On Oct 20, 2013, at 12:07 AM, [hidden email] wrote: > Modified: ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceLocation.java > URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceLocation.java?rev=1533839&r1=1533838&r2=1533839&view=diff > ============================================================================== > --- ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceLocation.java (original) > +++ ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceLocation.java Sat Oct 19 22:07:35 2013 > @@ -28,7 +28,7 @@ import org.w3c.dom.Element; > @ThreadSafe > public final class ServiceLocation { > > - private final String location; > + private String location; > private final String name; > > ServiceLocation(Element serviceLocationElement) throws ServiceConfigException { > @@ -48,6 +48,10 @@ public final class ServiceLocation { > return location; > } > > + public void setLocation(String location) { > + this.location = location; > + } > + > public String getName() { > return name; > } |
Re: svn commit: r1533839 - in /ofbiz/trunk: ./ framework/base/src/org/ofbiz/b
|
Administrator
|
I thought that since it's only used to modify a local List<ServiceLocation> variable in ServiceEngine constructor which is then put in
private final List<ServiceLocation> serviceLocations; of this class using this.serviceLocations = Collections.unmodifiableList(serviceLocations); there were no dangers I must say I did not consider external uses (not OOTB) of ServiceLocation.setLocation(). I guess there should not be. Jacques Le 12/09/2014 19:02, Jacopo Cappellato a écrit : > With this change you have broken the thread safety of the ServiceLocation.java class; where you aware of this? > > Jacopo > > On Oct 20, 2013, at 12:07 AM, [hidden email] wrote: > >> Modified: ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceLocation.java >> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceLocation.java?rev=1533839&r1=1533838&r2=1533839&view=diff >> ============================================================================== >> --- ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceLocation.java (original) >> +++ ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceLocation.java Sat Oct 19 22:07:35 2013 >> @@ -28,7 +28,7 @@ import org.w3c.dom.Element; >> @ThreadSafe >> public final class ServiceLocation { >> >> - private final String location; >> + private String location; >> private final String name; >> >> ServiceLocation(Element serviceLocationElement) throws ServiceConfigException { >> @@ -48,6 +48,10 @@ public final class ServiceLocation { >> return location; >> } >> >> + public void setLocation(String location) { >> + this.location = location; >> + } >> + >> public String getName() { >> return name; >> } > > |
Re: svn commit: r1533839 - in /ofbiz/trunk: ./ framework/base/src/org/ofbiz/b
|
|
On Sep 12, 2014, at 7:24 PM, Jacques Le Roux <[hidden email]> wrote: > this.serviceLocations = Collections.unmodifiableList(serviceLocations); Exactly: this pattern can only work under the assumption of the immutability of the serviceLocation objects; since you broke it the classes are no more thread safe. Jacopo |
Re: svn commit: r1533839 - in /ofbiz/trunk: ./ framework/base/src/org/ofbiz/b
|
Administrator
|
Le 12/09/2014 19:33, Jacopo Cappellato a écrit : > On Sep 12, 2014, at 7:24 PM, Jacques Le Roux<[hidden email]> wrote: > >> this.serviceLocations = Collections.unmodifiableList(serviceLocations); > Exactly: this pattern can only work under the assumption of the immutability of the serviceLocation objects; since you broke it the classes are no more thread safe. > > Jacopo > Immutability is not required, as long as we can guarantee that all threads will always have the same services engines locations values. BTW from a practical perspective this only had an impact when the -Dportoffset parameter was used. Else the services engines locations were actually not changed, once read from their "service-location" I suggest to synchronize the content of the else code block which begins at List<ServiceLocation> serviceLocations = new ArrayList<ServiceLocation>(serviceLocationElementList.size()); and ends at this.serviceLocations = Collections.unmodifiableList(serviceLocations); Since ServiceLocation.setLocation() is only used there (in OOTB code at least) we would be sure that all threads will always have the right values, even if the -Dportoffset parameter is used. The performance impact should not be huge. I guess initializing service engines is not often done, mostly (if not only, I could not verify completly) at start. Jacques |
Re: svn commit: r1533839 - in /ofbiz/trunk: ./ framework/base/src/org/ofbiz/b
|
|
If you are suggesting to synchronize access to the location field during writing but not during reading then this is wrong approach (this is the very first basic concept you should know when you touch a thread-safe class).
With this commit you have introduced a regression, and also the inline documentation (see the @ThreadSafe tag) of the ServiceLocation class is now misleading. And please no, don't do further work on this (unless you would like to revert your commit), it is better for OFBiz and the community if I take care of fixing this stuff. Jacopo On Sep 13, 2014, at 12:15 AM, Jacques Le Roux <[hidden email]> wrote: > > Le 12/09/2014 19:33, Jacopo Cappellato a écrit : >> On Sep 12, 2014, at 7:24 PM, Jacques Le Roux<[hidden email]> wrote: >> >>> this.serviceLocations = Collections.unmodifiableList(serviceLocations); >> Exactly: this pattern can only work under the assumption of the immutability of the serviceLocation objects; since you broke it the classes are no more thread safe. >> >> Jacopo >> > > Immutability is not required, as long as we can guarantee that all threads will always have the same services engines locations values. > BTW from a practical perspective this only had an impact when the -Dportoffset parameter was used. Else the services engines locations were actually not changed, once read from their "service-location" > > I suggest to synchronize the content of the else code block which begins at > List<ServiceLocation> serviceLocations = new ArrayList<ServiceLocation>(serviceLocationElementList.size()); > and ends at > this.serviceLocations = Collections.unmodifiableList(serviceLocations); > > Since ServiceLocation.setLocation() is only used there (in OOTB code at least) we would be sure that all threads will always have the right values, even if the -Dportoffset parameter is used. > The performance impact should not be huge. I guess initializing service engines is not often done, mostly (if not only, I could not verify completly) at start. > > Jacques |
Re: svn commit: r1533839 - in /ofbiz/trunk: ./ framework/base/src/org/ofbiz/b
|
|
In reply to this post by Jacques Le Roux
On Sep 12, 2014, at 6:16 PM, Jacques Le Roux <[hidden email]> wrote:
> It was the easiest way I found to pass it everywhere I needed to change the port I hope you'll understand that this is not a valid argument... especially from a person, like you, that is an OFBiz committer since a long time. By the way, I spent time cleaning your code in rev. 1624700. However the regressions that you have introduced with your commit are still there. Jacopo > > Jacques > > Le 12/09/2014 17:20, Jacopo Cappellato a écrit : >> Jacques, >> >> what was the rationale behind your design decision to add the portOffset field to the ClassLoaderContainer? Why ClassLoaderContainer? It doesn't make much sense to me... >> >> Jacopo >> >> On Oct 20, 2013, at 12:07 AM, [hidden email] wrote: >> >>> Author: jleroux >>> Date: Sat Oct 19 22:07:35 2013 >>> New Revision: 1533839 >>> >>> URL: http://svn.apache.org/r1533839 >>> Log: >>> Set different ports for testing in a CI environment (e.g. Jenkins) https://issues.apache.org/jira/browse/OFBIZ-4794 >>> >>> *What's for?* >>> Finally, OOTB the port offset is interesting in 2 cases: >>> # To eventually run Sonar on OFBiz sources https://issues.apache.org/jira/browse/INFRA-3590 >>> # To simultaneously run the demos without having to change much things, just one parameter (this will need to have the same commited in R13.07... to be discussed...) >>> >>> Of course, It can be used for other purposes outside of the ASF context... >>> >>> *How it's done?* >>> Basically, I just added a portoffset integer parameter to 3 ant targets (of course this JVM param can also be used in a Java call from command line): start, start-pos, start-both, >>> >>> The portoffset parameter is set to zero by default in ClassLoaderContainer.java. >>> If a value is passed, it's then grabed by Config.java and passed to ClassLoaderContainer.java, where it's set to this value. >>> >>> The portoffset is then used to change in one shoot all the default ports values defined (in config or hardcoded), and the admin port value >>> This is done in following classes: >>> CatalinaContainer >>> NamingServiceContainer >>> ServiceEngine through ServiceLocation >>> RmiServiceContainer >>> XMLRPCClientEngine >>> XmlRpcTests >>> >>> To avoid hardcoding locations in services definitions, I also created 2 service-locations: main-http and main-soap >>> >>> *To summarize:* these changes will be used to run "ant start" with a portoffset value which will take care of all ports values, including admin port. For running R12.04 demo as stable we will still need to create a patch for the last time. R13.07 could have a similar patch applied since it's not yet released... >>> >>> Modified: >>> ofbiz/trunk/build.xml >>> ofbiz/trunk/framework/base/src/org/ofbiz/base/container/ClassLoaderContainer.java >>> ofbiz/trunk/framework/base/src/org/ofbiz/base/container/NamingServiceContainer.java >>> ofbiz/trunk/framework/catalina/src/org/ofbiz/catalina/container/CatalinaContainer.java >>> ofbiz/trunk/framework/common/servicedef/services_test.xml >>> ofbiz/trunk/framework/service/config/serviceengine.xml >>> ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceEngine.java >>> ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceLocation.java >>> ofbiz/trunk/framework/service/src/org/ofbiz/service/engine/XMLRPCClientEngine.java >>> ofbiz/trunk/framework/service/src/org/ofbiz/service/rmi/RmiServiceContainer.java >>> ofbiz/trunk/framework/service/src/org/ofbiz/service/test/XmlRpcTests.java >>> ofbiz/trunk/framework/start/src/org/ofbiz/base/start/Config.java >>> >>> Modified: ofbiz/trunk/build.xml >>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/build.xml?rev=1533839&r1=1533838&r2=1533839&view=diff >>> ============================================================================== >>> --- ofbiz/trunk/build.xml (original) >>> +++ ofbiz/trunk/build.xml Sat Oct 19 22:07:35 2013 >>> @@ -408,11 +408,13 @@ under the License. >>> <!-- ================================================================== --> >>> >>> <target name="start" >>> - description="Start OFBiz"> >>> + description="Start OFBiz (use -Dportoffset=portNumber to shift all ports with the portNumber value)"> >>> <java jar="ofbiz.jar" fork="true"> >>> <jvmarg value="${memory.initial.param}"/> >>> <jvmarg value="${memory.max.param}"/> >>> <jvmarg value="${memory.maxpermsize.param}"/> >>> + <arg value="start"/> >>> + <arg value="-portoffset=${portoffset}"/> >>> </java> >>> </target> >>> <target name="start-batch" >>> @@ -436,21 +438,23 @@ under the License. >>> </java> >>> </target> >>> <target name="start-pos" >>> - description="Start OFBiz POS (Point of sale)"> >>> + description="Start OFBiz POS (Point of sale). Use -Dportoffset=portNumber to shift all ports with the portNumber value."> >>> <java jar="ofbiz.jar" fork="true"> >>> <jvmarg value="${memory.initial.param}"/> >>> <jvmarg value="${memory.max.param}"/> >>> <jvmarg value="${memory.maxpermsize.param}"/> >>> <arg value="pos"/> >>> + <arg value="-portoffset=${portoffset}"/> >>> </java> >>> </target> >>> <target name="start-both" >>> - description="Start OFBiz in both Web and POS (Point of sale) modes"> >>> + description="Start OFBiz in both Web and POS (Point of sale) modes. Use -Dportoffset=portNumber to shift all ports with the portNumber value."> >>> <java jar="ofbiz.jar" fork="true"> >>> <jvmarg value="${memory.initial.param}"/> >>> <jvmarg value="${memory.max.param}"/> >>> <jvmarg value="${memory.maxpermsize.param}"/> >>> <arg value="both"/> >>> + <arg value="-portoffset=${portoffset}"/> >>> </java> >>> </target> >>> <target name="stop" >>> @@ -1296,7 +1300,7 @@ under the License. >>> <!-- kek helper --> >>> <!-- ================================================================== --> >>> >>> - <target name="gen-kek" >>> + <target name="gen-kek" >>> description="Generate a new key-encrypting-key for use in entityengine.xml"> >>> <java classname="org.ofbiz.base.crypto.Main" fork="false"> >>> <arg value="-kek"/> >>> >>> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/container/ClassLoaderContainer.java >>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/container/ClassLoaderContainer.java?rev=1533839&r1=1533838&r2=1533839&view=diff >>> ============================================================================== >>> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/container/ClassLoaderContainer.java (original) >>> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/container/ClassLoaderContainer.java Sat Oct 19 22:07:35 2013 >>> @@ -18,11 +18,11 @@ >>> *******************************************************************************/ >>> package org.ofbiz.base.container; >>> >>> +import java.net.URL; >>> + >>> +import org.ofbiz.base.start.Classpath; >>> import org.ofbiz.base.util.CachedClassLoader; >>> import org.ofbiz.base.util.Debug; >>> -import org.ofbiz.base.start.Classpath; >>> - >>> -import java.net.URL; >>> >>> /** >>> * ClassLoader Container; Created a CachedClassLoader for use by all following containers >>> @@ -32,6 +32,7 @@ public class ClassLoaderContainer implem >>> >>> public static final String module = ClassLoaderContainer.class.getName(); >>> protected static CachedClassLoader cl = null; >>> + public static Integer portOffset = 0; >>> private String name; >>> >>> @Override >>> @@ -46,6 +47,34 @@ public class ClassLoaderContainer implem >>> } >>> >>> cl = new CachedClassLoader(new URL[0], parent); >>> + >>> + if (args != null) { >>> + for (String argument : args) { >>> + // arguments can prefix w/ a '-'. Just strip them off >>> + if (argument.startsWith("-")) { >>> + int subIdx = 1; >>> + if (argument.startsWith("--")) { >>> + subIdx = 2; >>> + } >>> + argument = argument.substring(subIdx); >>> + } >>> + >>> + // parse the arguments >>> + if (argument.indexOf("=") != -1) { >>> + String argumentName = argument.substring(0, argument.indexOf("=")); >>> + String argumentVal = argument.substring(argument.indexOf("=") + 1); >>> + >>> + if ("portoffset".equalsIgnoreCase(argumentName) && !"${portoffset}".equals(argumentVal)) { >>> + try { >>> + ClassLoaderContainer.portOffset = Integer.valueOf(argumentVal); >>> + } catch (NumberFormatException e) { >>> + e.printStackTrace(); >>> + } >>> + } >>> + } >>> + } >>> + } >>> + >>> Thread.currentThread().setContextClassLoader(cl); >>> Debug.logInfo("CachedClassLoader created", module); >>> } >>> >>> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/container/NamingServiceContainer.java >>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/container/NamingServiceContainer.java?rev=1533839&r1=1533838&r2=1533839&view=diff >>> ============================================================================== >>> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/container/NamingServiceContainer.java (original) >>> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/container/NamingServiceContainer.java Sat Oct 19 22:07:35 2013 >>> @@ -57,9 +57,9 @@ public class NamingServiceContainer impl >>> ContainerConfig.Container.Property port = cfg.getProperty("port"); >>> if (port.value != null) { >>> try { >>> - this.namingPort = Integer.parseInt(port.value); >>> + this.namingPort = Integer.parseInt(port.value) + ClassLoaderContainer.portOffset; >>> } catch (Exception e) { >>> - throw new ContainerException("Invalid port defined in container [naming-container] configuration; not a valid int"); >>> + throw new ContainerException("Invalid port defined in container [naming-container] configuration or as portOffset; not a valid int"); >>> } >>> } >>> >>> >>> Modified: ofbiz/trunk/framework/catalina/src/org/ofbiz/catalina/container/CatalinaContainer.java >>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/catalina/src/org/ofbiz/catalina/container/CatalinaContainer.java?rev=1533839&r1=1533838&r2=1533839&view=diff >>> ============================================================================== >>> --- ofbiz/trunk/framework/catalina/src/org/ofbiz/catalina/container/CatalinaContainer.java (original) >>> +++ ofbiz/trunk/framework/catalina/src/org/ofbiz/catalina/container/CatalinaContainer.java Sat Oct 19 22:07:35 2013 >>> @@ -246,6 +246,8 @@ public class CatalinaContainer implement >>> >>> for (Connector con: tomcat.getService().findConnectors()) { >>> ProtocolHandler ph = con.getProtocolHandler(); >>> + int port = con.getPort(); >>> + con.setAttribute("port", port); >>> if (ph instanceof Http11Protocol) { >>> Http11Protocol hph = (Http11Protocol) ph; >>> Debug.logInfo("Connector " + hph.getName() + " @ " + hph.getPort() + " - " + >>> @@ -477,7 +479,8 @@ public class CatalinaContainer implement >>> // need some standard properties >>> String protocol = ContainerConfig.getPropertyValue(connectorProp, "protocol", "HTTP/1.1"); >>> String address = ContainerConfig.getPropertyValue(connectorProp, "address", "0.0.0.0"); >>> - int port = ContainerConfig.getPropertyValue(connectorProp, "port", 0); >>> + int port = ContainerConfig.getPropertyValue(connectorProp, "port", 0) + ClassLoaderContainer.portOffset; >>> + >>> boolean secure = ContainerConfig.getPropertyValue(connectorProp, "secure", false); >>> if (protocol.toLowerCase().startsWith("ajp")) { >>> protocol = "ajp"; >>> @@ -537,8 +540,12 @@ public class CatalinaContainer implement >>> >>> try { >>> for (ContainerConfig.Container.Property prop: connectorProp.properties.values()) { >>> - connector.setProperty(prop.name, prop.value); >>> - //connector.setAttribute(prop.name, prop.value); >>> + if ("port".equals(prop.name)) { >>> + connector.setProperty(prop.name, "" + port); >>> + } else { >>> + connector.setProperty(prop.name, prop.value); >>> + //connector.setAttribute(prop.name, prop.value); >>> + } >>> } >>> >>> if (connectorProp.properties.containsKey("URIEncoding")) { >>> >>> Modified: ofbiz/trunk/framework/common/servicedef/services_test.xml >>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/common/servicedef/services_test.xml?rev=1533839&r1=1533838&r2=1533839&view=diff >>> ============================================================================== >>> --- ofbiz/trunk/framework/common/servicedef/services_test.xml (original) >>> +++ ofbiz/trunk/framework/common/servicedef/services_test.xml Sat Oct 19 22:07:35 2013 >>> @@ -72,21 +72,18 @@ under the License. >>> >>> <service name="groupTest" engine="group" location="testGroup" invoke=""/> >>> >>> - <service name="testHttp" engine="http" >>> - location="http://localhost:8080/webtools/control/httpService" invoke="testScv"> >>> + <service name="testHttp" engine="http" location="main-http" invoke="testScv"> >>> <description>HTTP service wrapper around the test service</description> >>> <attribute name="message" type="String" mode="IN" optional="true"/> >>> <attribute name="resp" type="String" mode="OUT"/> >>> </service> >>> >>> - <service name="testSoap" engine="soap" export="true" >>> - location="http://localhost:8080/webtools/control/SOAPService" invoke="testSOAPScv"> >>> + <service name="testSoap" engine="soap" export="true" location="main-soap" invoke="testSOAPScv"> >>> <description>SOAP service; calls the OFBiz test SOAP service</description> >>> <implements service="testSOAPScv"/> >>> </service> >>> >>> - <service name="testSoapSimple" engine="soap" export="true" >>> - location="http://localhost:8080/webtools/control/SOAPService" invoke="testScv"> >>> + <service name="testSoapSimple" engine="soap" export="true" location="main-soap" invoke="testScv"> >>> <description>simple SOAP service; calls the OFBiz test service</description> >>> <implements service="testScv"/> >>> </service> >>> >>> Modified: ofbiz/trunk/framework/service/config/serviceengine.xml >>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/service/config/serviceengine.xml?rev=1533839&r1=1533838&r2=1533839&view=diff >>> ============================================================================== >>> --- ofbiz/trunk/framework/service/config/serviceengine.xml (original) >>> +++ ofbiz/trunk/framework/service/config/serviceengine.xml Sat Oct 19 22:07:35 2013 >>> @@ -59,9 +59,8 @@ under the License. >>> <engine name="jms" class="org.ofbiz.service.jms.JmsServiceEngine"/> >>> <engine name="rmi" class="org.ofbiz.service.rmi.RmiServiceEngine"/> >>> <engine name="soap" class="org.ofbiz.service.engine.SOAPClientEngine"/> >>> - <!-- The engine xml-rpc-local is only used by a test service and for >>> - this reason it is configured to run on port 8080 (see rmi-dispatcher in service/ofbiz-component.xml); >>> - in order to use this in OFBiz change the port accordingly (for demo the default value is 8080) >>> + <!-- The engine xml-rpc-local is only used by a test service and for this reason it is configured to run on port 8080. >>> + In order to use this in OFBiz change the port accordingly (for demo the default value is 8080) >>> --> >>> <engine name="xml-rpc-local" class="org.ofbiz.service.engine.XMLRPCClientEngine"> >>> <parameter name="url" value="http://localhost:8080/webtools/control/xmlrpc"/> >>> @@ -71,7 +70,8 @@ under the License. >>> >>> <service-location name="main-rmi" location="rmi://localhost:1099/RMIDispatcher"/> >>> <service-location name="main-http" location="http://localhost:8080/webtools/control/httpService"/> >>> - >>> + <service-location name="main-soap" location="http://localhost:8080/webtools/control/SOAPService"/> >>> + >>> <service-location name="entity-sync-rmi" location="rmi://localhost:1099/RMIDispatcher"/> >>> <service-location name="entity-sync-http" location="http://localhost:8080/webtools/control/httpService"/> >>> >>> >>> Modified: ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceEngine.java >>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceEngine.java?rev=1533839&r1=1533838&r2=1533839&view=diff >>> ============================================================================== >>> --- ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceEngine.java (original) >>> +++ ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceEngine.java Sat Oct 19 22:07:35 2013 >>> @@ -24,6 +24,7 @@ import java.util.HashMap; >>> import java.util.List; >>> import java.util.Map; >>> >>> +import org.ofbiz.base.container.ClassLoaderContainer; >>> import org.ofbiz.base.lang.ThreadSafe; >>> import org.ofbiz.base.util.UtilXml; >>> import org.ofbiz.service.config.ServiceConfigException; >>> @@ -88,6 +89,16 @@ public final class ServiceEngine { >>> for (Element serviceLocationElement : serviceLocationElementList) { >>> serviceLocations.add(new ServiceLocation(serviceLocationElement)); >>> } >>> + for (ServiceLocation serviceLocation : serviceLocations) { >>> + String location = serviceLocation.getLocation(); >>> + if (location.contains("localhost") && ClassLoaderContainer.portOffset != 0) { >>> + Integer port = 1099 + ClassLoaderContainer.portOffset; >>> + location = location.replace("1099", port.toString()); >>> + port = 8080 + ClassLoaderContainer.portOffset; >>> + location = location.replace("8080", port.toString()); >>> + serviceLocation.setLocation(location); >>> + } >>> + } >>> this.serviceLocations = Collections.unmodifiableList(serviceLocations); >>> } >>> List<? extends Element> notificationGroupElementList = UtilXml.childElementList(engineElement, "notification-group"); >>> >>> Modified: ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceLocation.java >>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceLocation.java?rev=1533839&r1=1533838&r2=1533839&view=diff >>> ============================================================================== >>> --- ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceLocation.java (original) >>> +++ ofbiz/trunk/framework/service/src/org/ofbiz/service/config/model/ServiceLocation.java Sat Oct 19 22:07:35 2013 >>> @@ -28,7 +28,7 @@ import org.w3c.dom.Element; >>> @ThreadSafe >>> public final class ServiceLocation { >>> >>> - private final String location; >>> + private String location; >>> private final String name; >>> >>> ServiceLocation(Element serviceLocationElement) throws ServiceConfigException { >>> @@ -48,6 +48,10 @@ public final class ServiceLocation { >>> return location; >>> } >>> >>> + public void setLocation(String location) { >>> + this.location = location; >>> + } >>> + >>> public String getName() { >>> return name; >>> } >>> >>> Modified: ofbiz/trunk/framework/service/src/org/ofbiz/service/engine/XMLRPCClientEngine.java >>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/service/src/org/ofbiz/service/engine/XMLRPCClientEngine.java?rev=1533839&r1=1533838&r2=1533839&view=diff >>> ============================================================================== >>> --- ofbiz/trunk/framework/service/src/org/ofbiz/service/engine/XMLRPCClientEngine.java (original) >>> +++ ofbiz/trunk/framework/service/src/org/ofbiz/service/engine/XMLRPCClientEngine.java Sat Oct 19 22:07:35 2013 >>> @@ -28,6 +28,7 @@ import javolution.util.FastMap; >>> import org.apache.xmlrpc.XmlRpcException; >>> import org.apache.xmlrpc.client.XmlRpcClientConfigImpl; >>> import org.ofbiz.base.config.GenericConfigException; >>> +import org.ofbiz.base.container.ClassLoaderContainer; >>> import org.ofbiz.base.util.Debug; >>> import org.ofbiz.base.util.UtilGenerics; >>> import org.ofbiz.base.util.UtilMisc; >>> @@ -90,6 +91,10 @@ public class XMLRPCClientEngine extends >>> String keyAlias = null; >>> try { >>> url = ServiceConfigUtil.getEngineParameter(engine, "url"); >>> + if (ClassLoaderContainer.portOffset != 0) { >>> + Integer port = 8080 + ClassLoaderContainer.portOffset; >>> + url = url.replace("8080", port.toString()); >>> + } >>> login = ServiceConfigUtil.getEngineParameter(engine, "login"); >>> password = ServiceConfigUtil.getEngineParameter(engine, "password"); >>> keyStoreComponent = ServiceConfigUtil.getEngineParameter(engine, "keyStoreComponent"); >>> >>> Modified: ofbiz/trunk/framework/service/src/org/ofbiz/service/rmi/RmiServiceContainer.java >>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/service/src/org/ofbiz/service/rmi/RmiServiceContainer.java?rev=1533839&r1=1533838&r2=1533839&view=diff >>> ============================================================================== >>> --- ofbiz/trunk/framework/service/src/org/ofbiz/service/rmi/RmiServiceContainer.java (original) >>> +++ ofbiz/trunk/framework/service/src/org/ofbiz/service/rmi/RmiServiceContainer.java Sat Oct 19 22:07:35 2013 >>> @@ -26,6 +26,7 @@ import java.rmi.server.RMIServerSocketFa >>> import javax.naming.InitialContext; >>> import javax.naming.NamingException; >>> >>> +import org.ofbiz.base.container.ClassLoaderContainer; >>> import org.ofbiz.base.container.Container; >>> import org.ofbiz.base.container.ContainerConfig; >>> import org.ofbiz.base.container.ContainerException; >>> @@ -80,6 +81,11 @@ public class RmiServiceContainer impleme >>> String useCtx = initialCtxProp == null || initialCtxProp.value == null ? "false" : initialCtxProp.value; >>> String host = lookupHostProp == null || lookupHostProp.value == null ? "localhost" : lookupHostProp.value; >>> String port = lookupPortProp == null || lookupPortProp.value == null ? "1099" : lookupPortProp.value; >>> + if (ClassLoaderContainer.portOffset != 0) { >>> + Integer portValue = Integer.valueOf(port); >>> + portValue += ClassLoaderContainer.portOffset; >>> + port = portValue.toString(); >>> + } >>> String keystore = ContainerConfig.getPropertyValue(cfg, "ssl-keystore", null); >>> String ksType = ContainerConfig.getPropertyValue(cfg, "ssl-keystore-type", "JKS"); >>> String ksPass = ContainerConfig.getPropertyValue(cfg, "ssl-keystore-pass", null); >>> >>> Modified: ofbiz/trunk/framework/service/src/org/ofbiz/service/test/XmlRpcTests.java >>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/service/src/org/ofbiz/service/test/XmlRpcTests.java?rev=1533839&r1=1533838&r2=1533839&view=diff >>> ============================================================================== >>> --- ofbiz/trunk/framework/service/src/org/ofbiz/service/test/XmlRpcTests.java (original) >>> +++ ofbiz/trunk/framework/service/src/org/ofbiz/service/test/XmlRpcTests.java Sat Oct 19 22:07:35 2013 >>> @@ -23,6 +23,7 @@ import java.util.Locale; >>> import java.util.Map; >>> >>> import org.apache.xmlrpc.client.XmlRpcClient; >>> +import org.ofbiz.base.container.ClassLoaderContainer; >>> import org.ofbiz.base.util.UtilGenerics; >>> import org.ofbiz.base.util.UtilProperties; >>> import org.ofbiz.base.util.UtilValidate; >>> @@ -37,10 +38,14 @@ public class XmlRpcTests extends Abstrac >>> >>> public static final String module = XmlRpcTests.class.getName(); >>> public static final String resource = "ServiceErrorUiLabels"; >>> - public static final String url = "http://localhost:8080/webtools/control/xmlrpc"; >>> + public static String url = "http://localhost:8080/webtools/control/xmlrpc"; >>> >>> public XmlRpcTests(String name) { >>> super(name); >>> + if (ClassLoaderContainer.portOffset != 0) { >>> + Integer port = 8080 + ClassLoaderContainer.portOffset; >>> + url = url.replace("8080", port.toString()); >>> + } >>> } >>> >>> /** >>> >>> Modified: ofbiz/trunk/framework/start/src/org/ofbiz/base/start/Config.java >>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/start/src/org/ofbiz/base/start/Config.java?rev=1533839&r1=1533838&r2=1533839&view=diff >>> ============================================================================== >>> --- ofbiz/trunk/framework/start/src/org/ofbiz/base/start/Config.java (original) >>> +++ ofbiz/trunk/framework/start/src/org/ofbiz/base/start/Config.java Sat Oct 19 22:07:35 2013 >>> @@ -57,7 +57,7 @@ public class Config { >>> String firstArg = args.length > 0 ? args[0] : ""; >>> String configFileName = getConfigFileName(firstArg); >>> Config result = new Config(); >>> - result.readConfig(configFileName); >>> + result.readConfig(configFileName, args); >>> return result; >>> } >>> >>> @@ -283,7 +283,7 @@ public class Config { >>> } >>> } >>> >>> - public void readConfig(String config) throws IOException { >>> + public void readConfig(String config, String[] args) throws IOException { >>> // check the java_version >>> String javaVersion = System.getProperty("java.version"); >>> String javaVendor = System.getProperty("java.vendor"); >>> @@ -352,6 +352,14 @@ public class Config { >>> // parse the port number >>> try { >>> adminPort = Integer.parseInt(adminPortStr); >>> + if (args.length > 0) { >>> + for (String arg : args) { >>> + if (arg.toLowerCase().contains("portoffset=")) { >>> + adminPort = adminPort != 0 ? adminPort : 10523; // This is necessary because the ASF machines don't allow ports 1 to 3, see INFRA-6790 >>> + adminPort += Integer.parseInt(arg.split("=")[1]); >>> + } >>> + } >>> + } >>> } catch (Exception e) { >>> adminPort = 0; >>> } >>> @@ -396,16 +404,16 @@ public class Config { >>> // set the default locale >>> String localeString = props.getProperty("ofbiz.locale.default"); >>> if (localeString != null && localeString.length() > 0) { >>> - String args[] = localeString.split("_"); >>> - switch (args.length) { >>> + String locales[] = localeString.split("_"); >>> + switch (locales.length) { >>> case 1: >>> - Locale.setDefault(new Locale(args[0])); >>> + Locale.setDefault(new Locale(locales[0])); >>> break; >>> case 2: >>> - Locale.setDefault(new Locale(args[0], args[1])); >>> + Locale.setDefault(new Locale(locales[0], locales[1])); >>> break; >>> case 3: >>> - Locale.setDefault(new Locale(args[0], args[1], args[2])); >>> + Locale.setDefault(new Locale(locales[0], locales[1], args[2])); >>> } >>> System.setProperty("user.language", localeString); >>> } >>> >>> >> >> > |
Re: svn commit: r1533839 - in /ofbiz/trunk: ./ framework/base/src/org/ofbiz/b
|
Administrator
|
In reply to this post by Jacopo Cappellato-3
Le 13/09/2014 06:24, Jacopo Cappellato a écrit : > If you are suggesting to synchronize access to the location field during writing but not during reading then this is wrong approach (this is the very first basic concept you should know when you touch a thread-safe class). I checked this aspect (OOTB code only of course). ServiceLocation.getLocation() is only used in AbstractEngine.createLocationMap() where it depends on ServiceConfigUtil.getServiceEngine().getServiceLocations(). Since ServiceEngine.serviceLocations is a Collections.unmodifiableList created in the synchronized block there are no read threats, that's the purpose of the unmodifiableList. Immutability is the best solution to guarantee threadSafe, but it's not the only one. > With this commit you have introduced a regression, and also the inline documentation (see the @ThreadSafe tag) of the ServiceLocation class is now misleading. Ha indeed I missed the @ThreadSafe tag :/ Unfortunately with my solution it should be removed, and even a comment should be added. > > And please no, don't do further work on this (unless you would like to revert your commit), it is better for OFBiz and the community if I take care of fixing this stuff. I don't want to revert. I have created https://issues.apache.org/jira/browse/OFBIZ-5770 and attached a patch. Would you have a better solution which would keep ServiceLocation threadSafe? For other readers, again: this is currently only a possible problem if you use the -Dportoffset parameter. I never crossed issue personnaly and the official stable demo is running with it for few months w/o noticed issues. Jacques > > Jacopo > > On Sep 13, 2014, at 12:15 AM, Jacques Le Roux <[hidden email]> wrote: > >> Le 12/09/2014 19:33, Jacopo Cappellato a écrit : >>> On Sep 12, 2014, at 7:24 PM, Jacques Le Roux<[hidden email]> wrote: >>> >>>> this.serviceLocations = Collections.unmodifiableList(serviceLocations); >>> Exactly: this pattern can only work under the assumption of the immutability of the serviceLocation objects; since you broke it the classes are no more thread safe. >>> >>> Jacopo >>> >> Immutability is not required, as long as we can guarantee that all threads will always have the same services engines locations values. >> BTW from a practical perspective this only had an impact when the -Dportoffset parameter was used. Else the services engines locations were actually not changed, once read from their "service-location" >> >> I suggest to synchronize the content of the else code block which begins at >> List<ServiceLocation> serviceLocations = new ArrayList<ServiceLocation>(serviceLocationElementList.size()); >> and ends at >> this.serviceLocations = Collections.unmodifiableList(serviceLocations); >> >> Since ServiceLocation.setLocation() is only used there (in OOTB code at least) we would be sure that all threads will always have the right values, even if the -Dportoffset parameter is used. >> The performance impact should not be huge. I guess initializing service engines is not often done, mostly (if not only, I could not verify completly) at start. >> >> Jacques > > |
Re: svn commit: r1533839 - in /ofbiz/trunk: ./ framework/base/src/org/ofbiz/b
|
|
On Sep 13, 2014, at 11:14 AM, Jacques Le Roux <[hidden email]> wrote: > > Le 13/09/2014 06:24, Jacopo Cappellato a écrit : >> If you are suggesting to synchronize access to the location field during writing but not during reading then this is wrong approach (this is the very first basic concept you should know when you touch a thread-safe class). > > I checked this aspect (OOTB code only of course). ServiceLocation.getLocation() is only used in AbstractEngine.createLocationMap() where it depends on ServiceConfigUtil.getServiceEngine().getServiceLocations(). > Since ServiceEngine.serviceLocations is a Collections.unmodifiableList created in the synchronized block there are no read threats, that's the purpose of the unmodifiableList. > > Immutability is the best solution to guarantee threadSafe, but it's not the only one. Are you teaching me how to write thread safe code? :-) Jacques, the list in unmodifiable but the objects in it are modifiable (because of your commit): this breaks the pattern. > >> With this commit you have introduced a regression, and also the inline documentation (see the @ThreadSafe tag) of the ServiceLocation class is now misleading. > > Ha indeed I missed the @ThreadSafe tag :/ > Unfortunately with my solution it should be removed, and even a comment should be added. > >> >> And please no, don't do further work on this (unless you would like to revert your commit), it is better for OFBiz and the community if I take care of fixing this stuff. > > I don't want to revert. I have created https://issues.apache.org/jira/browse/OFBIZ-5770 and attached a patch. Your patch is a wrong approach: if you synchronize the writes but you do not synchronize the reads the code is still not thread safe; I already told you this but you don't read carefully what I am writing you and you keep replying with incorrect statements. > Would you have a better solution which would keep ServiceLocation threadSafe? yes, of course I do; I have committed it with rev. 1624767 > > For other readers, again: this is currently only a possible problem if you use the -Dportoffset parameter. I never crossed issue personnaly and the official stable demo is running with it for few months w/o noticed issues. The demo instances with the low traffic they experience aren't a good test bed for testing how the system deal with concurrent threads. The portOffset code you have committed in OFBiz has hardcoded port numbers and works only if you run it with the default configuration; it should be reverted completely. Jacopo > > Jacques > >> >> Jacopo >> >> On Sep 13, 2014, at 12:15 AM, Jacques Le Roux <[hidden email]> wrote: >> >>> Le 12/09/2014 19:33, Jacopo Cappellato a écrit : >>>> On Sep 12, 2014, at 7:24 PM, Jacques Le Roux<[hidden email]> wrote: >>>> >>>>> this.serviceLocations = Collections.unmodifiableList(serviceLocations); >>>> Exactly: this pattern can only work under the assumption of the immutability of the serviceLocation objects; since you broke it the classes are no more thread safe. >>>> >>>> Jacopo >>>> >>> Immutability is not required, as long as we can guarantee that all threads will always have the same services engines locations values. >>> BTW from a practical perspective this only had an impact when the -Dportoffset parameter was used. Else the services engines locations were actually not changed, once read from their "service-location" >>> >>> I suggest to synchronize the content of the else code block which begins at >>> List<ServiceLocation> serviceLocations = new ArrayList<ServiceLocation>(serviceLocationElementList.size()); >>> and ends at >>> this.serviceLocations = Collections.unmodifiableList(serviceLocations); >>> >>> Since ServiceLocation.setLocation() is only used there (in OOTB code at least) we would be sure that all threads will always have the right values, even if the -Dportoffset parameter is used. >>> The performance impact should not be huge. I guess initializing service engines is not often done, mostly (if not only, I could not verify completly) at start. >>> >>> Jacques >> >> > |
Re: svn commit: r1533839 - in /ofbiz/trunk: ./ framework/base/src/org/ofbiz/b
|
|
I recall when this feature was proposed, there was some opposition to
it. I had suggested using patches instead of making code modifications, but that idea was rejected. Adrian Crum Sandglass Software www.sandglass-software.com On 9/13/2014 6:20 PM, Jacopo Cappellato wrote: > > On Sep 13, 2014, at 11:14 AM, Jacques Le Roux <[hidden email]> wrote: > >> >> Le 13/09/2014 06:24, Jacopo Cappellato a écrit : >>> If you are suggesting to synchronize access to the location field during writing but not during reading then this is wrong approach (this is the very first basic concept you should know when you touch a thread-safe class). >> >> I checked this aspect (OOTB code only of course). ServiceLocation.getLocation() is only used in AbstractEngine.createLocationMap() where it depends on ServiceConfigUtil.getServiceEngine().getServiceLocations(). >> Since ServiceEngine.serviceLocations is a Collections.unmodifiableList created in the synchronized block there are no read threats, that's the purpose of the unmodifiableList. >> >> Immutability is the best solution to guarantee threadSafe, but it's not the only one. > > Are you teaching me how to write thread safe code? :-) > Jacques, the list in unmodifiable but the objects in it are modifiable (because of your commit): this breaks the pattern. > >> >>> With this commit you have introduced a regression, and also the inline documentation (see the @ThreadSafe tag) of the ServiceLocation class is now misleading. >> >> Ha indeed I missed the @ThreadSafe tag :/ >> Unfortunately with my solution it should be removed, and even a comment should be added. >> >>> >>> And please no, don't do further work on this (unless you would like to revert your commit), it is better for OFBiz and the community if I take care of fixing this stuff. >> >> I don't want to revert. I have created https://issues.apache.org/jira/browse/OFBIZ-5770 and attached a patch. > > Your patch is a wrong approach: if you synchronize the writes but you do not synchronize the reads the code is still not thread safe; I already told you this but you don't read carefully what I am writing you and you keep replying with incorrect statements. > >> Would you have a better solution which would keep ServiceLocation threadSafe? > > yes, of course I do; I have committed it with rev. 1624767 > >> >> For other readers, again: this is currently only a possible problem if you use the -Dportoffset parameter. I never crossed issue personnaly and the official stable demo is running with it for few months w/o noticed issues. > > The demo instances with the low traffic they experience aren't a good test bed for testing how the system deal with concurrent threads. > The portOffset code you have committed in OFBiz has hardcoded port numbers and works only if you run it with the default configuration; it should be reverted completely. > > Jacopo > >> >> Jacques >> >>> >>> Jacopo >>> >>> On Sep 13, 2014, at 12:15 AM, Jacques Le Roux <[hidden email]> wrote: >>> >>>> Le 12/09/2014 19:33, Jacopo Cappellato a écrit : >>>>> On Sep 12, 2014, at 7:24 PM, Jacques Le Roux<[hidden email]> wrote: >>>>> >>>>>> this.serviceLocations = Collections.unmodifiableList(serviceLocations); >>>>> Exactly: this pattern can only work under the assumption of the immutability of the serviceLocation objects; since you broke it the classes are no more thread safe. >>>>> >>>>> Jacopo >>>>> >>>> Immutability is not required, as long as we can guarantee that all threads will always have the same services engines locations values. >>>> BTW from a practical perspective this only had an impact when the -Dportoffset parameter was used. Else the services engines locations were actually not changed, once read from their "service-location" >>>> >>>> I suggest to synchronize the content of the else code block which begins at >>>> List<ServiceLocation> serviceLocations = new ArrayList<ServiceLocation>(serviceLocationElementList.size()); >>>> and ends at >>>> this.serviceLocations = Collections.unmodifiableList(serviceLocations); >>>> >>>> Since ServiceLocation.setLocation() is only used there (in OOTB code at least) we would be sure that all threads will always have the right values, even if the -Dportoffset parameter is used. >>>> The performance impact should not be huge. I guess initializing service engines is not often done, mostly (if not only, I could not verify completly) at start. >>>> >>>> Jacques >>> >>> >> > |
Re: svn commit: r1533839 - in /ofbiz/trunk: ./ framework/base/src/org/ofbiz/b
|
Administrator
|
In reply to this post by Jacopo Cappellato-3
Le 13/09/2014 19:20, Jacopo Cappellato a écrit : > On Sep 13, 2014, at 11:14 AM, Jacques Le Roux <[hidden email]> wrote: > >> Le 13/09/2014 06:24, Jacopo Cappellato a écrit : >>> If you are suggesting to synchronize access to the location field during writing but not during reading then this is wrong approach (this is the very first basic concept you should know when you touch a thread-safe class). >> I checked this aspect (OOTB code only of course). ServiceLocation.getLocation() is only used in AbstractEngine.createLocationMap() where it depends on ServiceConfigUtil.getServiceEngine().getServiceLocations(). >> Since ServiceEngine.serviceLocations is a Collections.unmodifiableList created in the synchronized block there are no read threats, that's the purpose of the unmodifiableList. >> >> Immutability is the best solution to guarantee threadSafe, but it's not the only one. > Are you teaching me how to write thread safe code? :-) I did not say that. I was just trying to find a solution and it appeared to me that nothing OOTB was modifying the content of the unmodifiableList once set, so it was safe. I was not considering OFBiz code as an API. I understand now someone could have used setLocation() in another place. That's where my reasoning was wrong. > Jacques, the list in unmodifiable but the objects in it are modifiable (because of your commit): this breaks the pattern. >>> With this commit you have introduced a regression, and also the inline documentation (see the @ThreadSafe tag) of the ServiceLocation class is now misleading. >> Ha indeed I missed the @ThreadSafe tag :/ >> Unfortunately with my solution it should be removed, and even a comment should be added. >> >>> And please no, don't do further work on this (unless you would like to revert your commit), it is better for OFBiz and the community if I take care of fixing this stuff. >> I don't want to revert. I have created https://issues.apache.org/jira/browse/OFBIZ-5770 and attached a patch. > Your patch is a wrong approach: if you synchronize the writes but you do not synchronize the reads the code is still not thread safe; I already told you this but you don't read carefully what I am writing you and you keep replying with incorrect statements. >> Would you have a better solution which would keep ServiceLocation threadSafe? > yes, of course I do; I have committed it with rev. 1624767 Too bad I did not thought about that, it's the right way indeed. I understand now I was blinded by my own error, when I introduced setLocation() >> For other readers, again: this is currently only a possible problem if you use the -Dportoffset parameter. I never crossed issue personnaly and the official stable demo is running with it for few months w/o noticed issues. > The demo instances with the low traffic they experience aren't a good test bed for testing how the system deal with concurrent threads. The demo instance was not a strong argument indeed. My point was nobody was at risk as long as s/he was not using the -Dportoffset parameter. setLocation() was then not used, so nothing changed. But I forgot the possible external usage (as an API) of setLocation(). > The portOffset code you have committed in OFBiz has hardcoded port numbers and works only if you run it with the default configuration; it should be reverted completely. I have fixed that at r1624809 in trunk and r1624814 in R13.07 after backporting r1624767 there at r1624812 Thanks for your patience Jacques > > Jacopo > >> Jacques >> >>> Jacopo >>> >>> On Sep 13, 2014, at 12:15 AM, Jacques Le Roux <[hidden email]> wrote: >>> >>>> Le 12/09/2014 19:33, Jacopo Cappellato a écrit : >>>>> On Sep 12, 2014, at 7:24 PM, Jacques Le Roux<[hidden email]> wrote: >>>>> >>>>>> this.serviceLocations = Collections.unmodifiableList(serviceLocations); >>>>> Exactly: this pattern can only work under the assumption of the immutability of the serviceLocation objects; since you broke it the classes are no more thread safe. >>>>> >>>>> Jacopo >>>>> >>>> Immutability is not required, as long as we can guarantee that all threads will always have the same services engines locations values. >>>> BTW from a practical perspective this only had an impact when the -Dportoffset parameter was used. Else the services engines locations were actually not changed, once read from their "service-location" >>>> >>>> I suggest to synchronize the content of the else code block which begins at >>>> List<ServiceLocation> serviceLocations = new ArrayList<ServiceLocation>(serviceLocationElementList.size()); >>>> and ends at >>>> this.serviceLocations = Collections.unmodifiableList(serviceLocations); >>>> >>>> Since ServiceLocation.setLocation() is only used there (in OOTB code at least) we would be sure that all threads will always have the right values, even if the -Dportoffset parameter is used. >>>> The performance impact should not be huge. I guess initializing service engines is not often done, mostly (if not only, I could not verify completly) at start. >>>> >>>> Jacques >>> > > |
Re: svn commit: r1533839 - in /ofbiz/trunk: ./ framework/base/src/org/ofbiz/b
|
Administrator
|
In reply to this post by Adrian Crum-3
I believe it's clean now
Jacques Le 13/09/2014 20:32, Adrian Crum a écrit : > I recall when this feature was proposed, there was some opposition to it. I had suggested using patches instead of making code modifications, but > that idea was rejected. > > Adrian Crum > Sandglass Software > www.sandglass-software.com > > On 9/13/2014 6:20 PM, Jacopo Cappellato wrote: >> >> On Sep 13, 2014, at 11:14 AM, Jacques Le Roux <[hidden email]> wrote: >> >>> >>> Le 13/09/2014 06:24, Jacopo Cappellato a écrit : >>>> If you are suggesting to synchronize access to the location field during writing but not during reading then this is wrong approach (this is the >>>> very first basic concept you should know when you touch a thread-safe class). >>> >>> I checked this aspect (OOTB code only of course). ServiceLocation.getLocation() is only used in AbstractEngine.createLocationMap() where it >>> depends on ServiceConfigUtil.getServiceEngine().getServiceLocations(). >>> Since ServiceEngine.serviceLocations is a Collections.unmodifiableList created in the synchronized block there are no read threats, that's the >>> purpose of the unmodifiableList. >>> >>> Immutability is the best solution to guarantee threadSafe, but it's not the only one. >> >> Are you teaching me how to write thread safe code? :-) >> Jacques, the list in unmodifiable but the objects in it are modifiable (because of your commit): this breaks the pattern. >> >>> >>>> With this commit you have introduced a regression, and also the inline documentation (see the @ThreadSafe tag) of the ServiceLocation class is >>>> now misleading. >>> >>> Ha indeed I missed the @ThreadSafe tag :/ >>> Unfortunately with my solution it should be removed, and even a comment should be added. >>> >>>> >>>> And please no, don't do further work on this (unless you would like to revert your commit), it is better for OFBiz and the community if I take >>>> care of fixing this stuff. >>> >>> I don't want to revert. I have created https://issues.apache.org/jira/browse/OFBIZ-5770 and attached a patch. >> >> Your patch is a wrong approach: if you synchronize the writes but you do not synchronize the reads the code is still not thread safe; I already >> told you this but you don't read carefully what I am writing you and you keep replying with incorrect statements. >> >>> Would you have a better solution which would keep ServiceLocation threadSafe? >> >> yes, of course I do; I have committed it with rev. 1624767 >> >>> >>> For other readers, again: this is currently only a possible problem if you use the -Dportoffset parameter. I never crossed issue personnaly and >>> the official stable demo is running with it for few months w/o noticed issues. >> >> The demo instances with the low traffic they experience aren't a good test bed for testing how the system deal with concurrent threads. >> The portOffset code you have committed in OFBiz has hardcoded port numbers and works only if you run it with the default configuration; it should >> be reverted completely. >> >> Jacopo >> >>> >>> Jacques >>> >>>> >>>> Jacopo >>>> >>>> On Sep 13, 2014, at 12:15 AM, Jacques Le Roux <[hidden email]> wrote: >>>> >>>>> Le 12/09/2014 19:33, Jacopo Cappellato a écrit : >>>>>> On Sep 12, 2014, at 7:24 PM, Jacques Le Roux<[hidden email]> wrote: >>>>>> >>>>>>> this.serviceLocations = Collections.unmodifiableList(serviceLocations); >>>>>> Exactly: this pattern can only work under the assumption of the immutability of the serviceLocation objects; since you broke it the classes are >>>>>> no more thread safe. >>>>>> >>>>>> Jacopo >>>>>> >>>>> Immutability is not required, as long as we can guarantee that all threads will always have the same services engines locations values. >>>>> BTW from a practical perspective this only had an impact when the -Dportoffset parameter was used. Else the services engines locations were >>>>> actually not changed, once read from their "service-location" >>>>> >>>>> I suggest to synchronize the content of the else code block which begins at >>>>> List<ServiceLocation> serviceLocations = new ArrayList<ServiceLocation>(serviceLocationElementList.size()); >>>>> and ends at >>>>> this.serviceLocations = Collections.unmodifiableList(serviceLocations); >>>>> >>>>> Since ServiceLocation.setLocation() is only used there (in OOTB code at least) we would be sure that all threads will always have the right >>>>> values, even if the -Dportoffset parameter is used. >>>>> The performance impact should not be huge. I guess initializing service engines is not often done, mostly (if not only, I could not verify >>>>> completly) at start. >>>>> >>>>> Jacques >>>> >>>> >>> >> > |
«
Return to OFBiz - Dev
|
1 view|%1 views
| Free forum by Nabble | Edit this page |