Re: svn commit: r652177 - in /ofbiz/trunk/framework: appserver/config/appserver.properties appserver/templates/wasce2/README base/src/base/org/ofbiz/base/container/ContainerLoader.java webapp/src/org/ofbiz/webapp/control/ContextFilter.java

[Jacopo Cappellato-4]

There are a series of issues in this commit.
I know this is a rather old one, and a review at this point may seem too late. But since, as we discussed in the dev list recently, I am going to revert most of this code today, I wanted to highlight some of the reasons more clearly.
Please see my remarks inline:

On Apr 30, 2008, at 12:35 AM, [hidden email] wrote:

> Author: jleroux
> Date: Tue Apr 29 15:35:32 2008
> New Revision: 652177
>
> URL: http://svn.apache.org/viewvc?rev=652177&view=rev
> Log:
> This add the possiblity to run RMI services under WASCE or Geronimo.
> There is only one small drawback : so far if you restart the OFBiz EAR from the console you loose the RMI Dispatcher. However stopping and starting (even after a restart) is OK. I guess it's the same for the commande redeploy, I will test tomorrow.
>
> Modified:
>    ofbiz/trunk/framework/appserver/config/appserver.properties
>    ofbiz/trunk/framework/appserver/templates/wasce2/README
>    ofbiz/trunk/framework/base/src/base/org/ofbiz/base/container/ContainerLoader.java
>    ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/ContextFilter.java
>
> Modified: ofbiz/trunk/framework/appserver/config/appserver.properties
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/appserver/config/appserver.properties?rev=652177&r1=652176&r2=652177&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/appserver/config/appserver.properties (original)
> +++ ofbiz/trunk/framework/appserver/config/appserver.properties Tue Apr 29 15:35:32 2008
> @@ -24,10 +24,12 @@
> ### Linux
> #geronimoHome=/home/jacques/IBM/WebSphere/AppServerCommunityEdition
> #geronimoHome=/home/jacques/Apache Software Foundation/geronimo-tomcat6-jee5-2.0.3
> -### Windows *** Note that under Windows you must use / here in place of \ (or put \\) ***
> -#geronimoHome=C:/Program Files/IBM/WebSphere/AppServerCommunityEdition
> -# Beware for geronimo-2.0.3 snapshot, under Windows some files have a path longer than 256 chars. Hence I put it directly under c:
> -#geronimoHome=C:/geronimo-tomcat6-jee5-2.0.3
> +### Windows
> +#*** Note that under Windows you must use / here in place of \ (or put \\)
> +#*** Beware for geronimo-2.0.3 snapshot, under Windows some files have a path longer than 256 chars. Hence I put it directly under c:
> +#*** Beware also if you use RMI you can't have spaces in path
> +#geronimoHome=C:/wasce
> +#geronimoHome=C:/geronimo-tomcat6-jee5-2.0.3
>
> ### user login
> user=system
>
> Modified: ofbiz/trunk/framework/appserver/templates/wasce2/README
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/appserver/templates/wasce2/README?rev=652177&r1=652176&r2=652177&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/appserver/templates/wasce2/README (original)
> +++ ofbiz/trunk/framework/appserver/templates/wasce2/README Tue Apr 29 15:35:32 2008
> @@ -73,4 +73,26 @@
> Replace the line
>     -jar "$GERONIMO_HOME"/bin/server.jar $LONG_OPT "$@"
> by
> -    -classpath ".":"$GERONIMO_HOME"/bin/server.jar:"$CLASSPATH" org.apache.geronimo.cli.daemon.DaemonCLI $LONG_OPT "$@"
> \ No newline at end of file
> +    -classpath ".":"$GERONIMO_HOME"/bin/server.jar:"$CLASSPATH" org.apache.geronimo.cli.daemon.DaemonCLI $LONG_OPT "$@"
> +
> +
> +=======================================================================================================================================================
> +    If you want to use RMI (RMIDispatcher) you will need to put the following lines in the limited-containers.xml file (set the values as you want)
> +        <property name="bound-host" value="127.0.0.1"/>
> +        <property name="bound-port" value="1099"/>
> +
> +    You will need also to change the value of use-initial-context to false and to put the complete path for ssl-keystore.
> +
> +    Finally you will have to pass a client policy to the application server using the -Djava.security.policy=client.policy parameter,
> +    simply put it at the end of JAVA_OPTS above.
> +    Indeed to load the RMIDispatcher under the application server it needs a RMI Security Manager.
> +    For my test I used a very simple client.policy file with all permissions that I put in the bin directory of the application server.
> +    Its content is :
> +        grant{
> +        permission java.security.AllPermission;
> +        };
> +
> +    *** Also beware, you can't have spaces in the path where the application server is installed ***
> +    And last but not least you may appreciate http://docs.ofbiz.org/display/OFBIZ/FAQ+-+Tips+-+Tricks+-+Cookbook+-+HowTo#FAQ-Tips-Tricks-Cookbook-HowTo-HowtouseRMIwithoutSSL
> +=======================================================================================================================================================
> +
>
> Modified: ofbiz/trunk/framework/base/src/base/org/ofbiz/base/container/ContainerLoader.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/base/org/ofbiz/base/container/ContainerLoader.java?rev=652177&r1=652176&r2=652177&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/base/src/base/org/ofbiz/base/container/ContainerLoader.java (original)
> +++ ofbiz/trunk/framework/base/src/base/org/ofbiz/base/container/ContainerLoader.java Tue Apr 29 15:35:32 2008
> @@ -42,6 +42,7 @@
>
>     protected List<Container> loadedContainers = new LinkedList<Container>();
>     protected String configFile = null;
> +    public static Container rmiLoadedContainer = null; // used in Geronimo/WASCE to allow to deregister
>
>     /**
>      * @see org.ofbiz.base.start.StartupLoader#load(Start.Config, String[])
> @@ -62,7 +63,28 @@

The code that follows is really an hack, based on hardcoded container and file names; makes the code difficult to read; most of all it breaks a contract: it starts a container from the load method, instead of simply loading it as it does with all the other containers.

>
>         if (containers != null) {
>             for (ContainerConfig.Container containerCfg: containers) {
> -                loadedContainers.add(loadContainer(containerCfg, args));
> +                Container tmpContainer = loadContainer(containerCfg, args);
> +                loadedContainers.add(tmpContainer);
> +
> +                // This is only used in case of OFBiz running in Geronimo or WASCE. It allows to use the RMIDispatcher
> +                if (containerCfg.name.equals("rmi-dispatcher") && configFile.equals("limited-containers.xml")) {
> +                    try {
> +                        ContainerConfig.Container.Property initialCtxProp = containerCfg.getProperty("use-initial-context");
> +                        String useCtx = initialCtxProp == null || initialCtxProp.value == null ? "false" : initialCtxProp.value;
> +                        if (!useCtx.equalsIgnoreCase("true")) {
> +                            //system.setProperty("java.security.policy", "client.policy"); maybe used if needed...
> +                            if (System.getSecurityManager() == null) { // needed by WASCE with a client.policy file.
> +                                System.setSecurityManager(new java.rmi.RMISecurityManager());
> +                            }
> +                            tmpContainer.start();
> +                            rmiLoadedContainer = tmpContainer; // used in Geronimo/WASCE to allow to deregister
> +                        }
> +                    } catch (ContainerException e) {
> +                        throw new StartupException("Cannot start() " + tmpContainer.getClass().getName(), e);
> +                    } catch (java.lang.AbstractMethodError e) {
> +                        throw new StartupException("Cannot start() " + tmpContainer.getClass().getName(), e);
> +                    }
> +                }
>             }
>         }
>     }
> @@ -175,14 +197,16 @@
>         return containerObj;
>     }
>

The following code makes the loadContainers method really ugly:

> -    public static synchronized boolean loadContainers(String config, String[] args) throws StartupException {
> +    public static synchronized Container loadContainers(String config, String[] args) throws StartupException {
>         if (!loaded) {
>             ContainerLoader loader = new ContainerLoader();
>             Start.Config cfg = new Start.Config();
>             cfg.containerConfig = config == null ? "limited-containers.xml" : config;
>             loader.load(cfg, args);
> -            return true;
> +            if (rmiLoadedContainer != null) { // used in Geronimo/WASCE to allow to deregister
> +                return rmiLoadedContainer;
> +            }
>         }
> -        return false;
> +        return null;
>     }
> }
>
> Modified: ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/ContextFilter.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/ContextFilter.java?rev=652177&r1=652176&r2=652177&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/ContextFilter.java (original)
> +++ ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/ContextFilter.java Tue Apr 29 15:35:32 2008
> @@ -37,6 +37,8 @@
> import javax.servlet.http.HttpServletResponse;
> import javax.servlet.http.HttpServletResponseWrapper;
>
> +import org.ofbiz.base.container.Container;
> +import org.ofbiz.base.container.ContainerException;
> import org.ofbiz.base.container.ContainerLoader;
> import org.ofbiz.base.start.StartupException;
> import org.ofbiz.base.util.CachedClassLoader;
> @@ -64,6 +66,8 @@
>     protected ClassLoader localCachedClassLoader = null;
>     protected FilterConfig config = null;
>     protected boolean debug = false;

The changes below are causing some issues during shutdown: the system attempts to shutdown the RMIContainer several times; they also add a lot of Geronimo specific code to a framework class.

Regards,

Jacopo

> +    protected Container rmiLoadedContainer = null; // used in Geronimo/WASCE to allow to deregister
> +
>
>     /**
>      * @see javax.servlet.Filter#init(javax.servlet.FilterConfig)
> @@ -85,7 +89,10 @@
>         }
>
>         // load the containers
> -        getContainers();
> +        Container container = getContainers();
> +        if (container != null) {
> +            rmiLoadedContainer = container; // used in Geronimo/WASCE to allow to deregister
> +        }
>         // check the serverId
>         getServerId();
>         // initialize the delegator
> @@ -253,6 +260,11 @@
>      */
>     public void destroy() {
>         getDispatcher().deregister();
> +        try {
> +            destroyRmiContainer(); // used in Geronimo/WASCE to allow to deregister
> +        } catch (ServletException e) {
> +            Debug.logError("Error when stopping containers, this exception should not arise...", module);
> +        }
>         config = null;
>     }
>
> @@ -366,13 +378,26 @@
>         return serverId;
>     }
>
> -    protected boolean getContainers() throws ServletException {
> +    protected Container getContainers() throws ServletException {
> +        Container rmiLoadedContainer = null;
>         try {
> -            ContainerLoader.loadContainers(CONTAINER_CONFIG, null);
> +            rmiLoadedContainer = ContainerLoader.loadContainers(CONTAINER_CONFIG, null); // used in Geronimo/WASCE to allow to deregister
>         } catch (StartupException e) {
>             Debug.logError(e, module);
>             throw new ServletException("Unable to load containers; cannot start ContextFilter");
>         }
> -        return true;
> +        return rmiLoadedContainer;
> +    }
> +
> +    // used in Geronimo/WASCE to allow to deregister
> +    protected void destroyRmiContainer() throws ServletException {
> +        if (rmiLoadedContainer != null) {
> +            try {
> +                rmiLoadedContainer.stop();
> +            } catch (ContainerException e) {
> +                Debug.logError(e, module);
> +                throw new ServletException("Error when stopping the RMI loaded container");
> +            }
> +        }
>     }
> }
>
>