geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark <delafran_j...@hotmail.com>
Subject Re: Review - svn commit: r126264
Date Tue, 25 Jan 2005 03:28:58 GMT
Thanks for the review.  I will go through your comments one-by-one over 
the next few days.

A few things to keep in mind:

1. The rmi/iiop code is fairly solid (with my fingers crossed).  I hope 
to get in a bunch of test cases that following the Geronimo design.
2. The stub/skeleton generation is farily *dirty* it.  You can convience 
it to work, code generation style/speed were not my primary objective.  
The rmi-iiop was.  So I fully anticipate this to change in the near future.
3. The original code was stand alone, hence there are a few other 
interop.* packages.  Once I learn more about Geronimo, I anticipate a 
much neater/tighter integration.  Of course, I think I will have lots of 
help from Alan.  :-)
4. I don't mind making the code or style changes to conform tothe Apache 
project.  The current style is a required habit for other projects that 
I work on.  :-)

I've added a few quick answers below see MD>>

( I only made it 1/2 way through this email... )

Expect that I will be submitting lots more changes to Alan over the 
comming days.

Thanks
Mark

Dain Sundstrom wrote:

> Mark,
>
> Wow!  This is very impressive work. I spent some time reading over 
> this  (well the first half... the thing is huge).  Most of the 
> comments, I  have are just my curiosity and don't really need to be 
> fixed.  Other  are notes on differences between this code an geronimo, 
> and I these  cases I think we should change current geronimo or this 
> new code so we  don't have multiple ways to do the same thing (BTW, I 
> generally don't  have a preference on which code we change unless one 
> is broken :)  Then  there are the few coding convention things, and 
> finally there are a few  nit picky things :)
>
> Thanks for the great work,
>
> -dain
>
>
> On Jan 23, 2005, at 11:33 PM, adc@apache.org wrote:
>
>> Added: geronimo/trunk/modules/interop/src/idl/CosNaming.idl
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/idl/ 
>> CosNaming.idl?view=auto&rev=126264
>>
>> Added: geronimo/trunk/modules/interop/src/idl/GIOP.idl
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/idl/ 
>> GIOP.idl?view=auto&rev=126264
>>
>> Added: geronimo/trunk/modules/interop/src/idl/IIOP.idl
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/idl/ 
>> IIOP.idl?view=auto&rev=126264
>>
>> Added: geronimo/trunk/modules/interop/src/idl/IOP.idl
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/idl/ 
>> IOP.idl?view=auto&rev=126264
>>
>> Added:  
>> geronimo/trunk/modules/interop/src/idl/org-apache-geronimo-interop- 
>> rmi-iiop.idl
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/idl/ 
>> org-apache-geronimo-interop-rmi-iiop.idl?view=auto&rev=126264
>
>
> The idl files need Apache license headers... maybe in the comments 
> that  are copied into the generated code.


MD>> If these are taken from the www.omg.org website, should we include 
the apache headers?

>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> CheckedException.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/CheckedException.java?view=auto&rev=126264
>>
>> +/**
>> + * * A wrapper that allows checked exceptions to be thrown as  
>> unchecked.
>> + */
>> +public class CheckedException extends RuntimeException {
>> +    public CheckedException(Throwable cause) {
>> +        super(cause);
>> +    }
>> +}
>
>
> How is this used?  Normally in Geronimo we have our invocation chains  
> (interceptor stacks) throw Throwable, and we just let exceptions flow  
> up.  In other cases, we divide exceptions into System and 
> Application.   We let the System exceptions flow up and the 
> Application exceptions are  wrapped in the results object since they 
> are treated as a normal return  for ejbs.  Anyway, I'm just curious.  
> If it falls into one of the basic  scenarios we already have a 
> preferred solution for, I'd like to either  switch the exiting code to 
> follow the new pattern or switch the new  iiop code to follow the 
> current pattern.


MD>> Its most likely that this class will go away soon.

>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> CosNaming/iiop_stubs/NamingContext_Stub.1.txt
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/CosNaming/iiop_stubs/ 
>> NamingContext_Stub.1.txt?view=auto&rev=126264
>>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> CosNaming/iiop_stubs/NamingContext_Stub.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/CosNaming/iiop_stubs/ 
>> NamingContext_Stub.java?view=auto&rev=126264
>
>
> Are these generated stubs based on the IDL above?  Normally, we don't  
> check in generated code, but I bet these almost never change.


MD>> The name service stubs/skels are generated.  Right now, due to a 
couple of issues, I have hand modified them and simply checked them in.  
This will need to change.  All other generated files go under 
interop/target/src


>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> InteropGBean.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/InteropGBean.java?view=auto&rev=126264
>> ======================================================================= 
>> =======
>> --- (empty file)
>> +++  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> InteropGBean.java    Sun Jan 23 23:33:10 2005
>>
>> +/**
>> + * A GBean that provides an example interop
>> + *
>> + * @version $Rev: $ $Date: $
>> + */
>> +public class InteropGBean implements GBeanLifecycle {
>
>
> Is this test code?  If it is an example we want to ship with the  
> server, maybe we should put it in an example package or rename it  
> ExampleInteropGBean.


MD>> This GBean was the interop/corba container loader.  maybe a better 
name is in order.  I haven't had a chance to flesh out the details for 
the Geronimo integration.  I'd like to see the interop container be 
loaded by the InteropGBean.  The InteropGBean could be a seperate plan 
or as part of the j2ee-server plan.  For the J2EE 1.4 configuration, the 
interop container would be enabled, but if a customer didn't care about 
the interop container they could remove the plan from the server 
configuration.

>
>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> SystemException.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/SystemException.java?view=auto&rev=126264
>
>
> Same comment as above for CheckedException.
>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> adapter/Adapter.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/adapter/Adapter.java?view=auto&rev=126264
>> ======================================================================= 
>> =======
>> +public class Adapter {
>> +    //
>> +    // Public Accessible Properties
>> +    //
>
>
> I don't think we need comments like this... it is self explanatory.


MD>>  Default template code .. Comments can be taken out. :-)

>
>
>> +    protected String _bindName;
>> +    protected String _remoteClassName;
>> +    protected String _remoteInterfaceName;
>> +    protected Vector _idVector;
>> +    protected boolean _shared;
>> +    protected ClassLoader _cl;
>> +    protected RemoteInterface _ri;
>
>
> Our naming convention doesn't use lead underscores.  Also we tend not  
> to use protected fields.  Maybe there is a subclass or some reason.


MD>> I have noticed that.  Will be changed.

>
>> +    //
>> +    // Internal Properrties
>> +    //
>> +
>> +    protected Object _sharedObject;
>> +    protected HashMap _objects;
>> +    protected Class _remoteClassClass;
>> +    protected Class _remoteInterfaceClass;
>> +
>> +    public Adapter() {
>> +        _objects = new HashMap();
>> +        _idVector = new Vector();
>> +    }
>> +
>> +    /*
>> +     * BindName is the name that will be registered with the INS  
>> (Inter-operable Name Service)
>> +     */
>> +    public String getBindName() {
>> +        return _bindName;
>> +    }
>> +
>> +    public void setBindName(String bindName) {
>> +        _bindName = bindName;
>> +    }
>> +
>> +    /*
>> +     * Is this a shared component?  If so this will invoke the  
>> getInstance method on
>> +     * the component ...
>> +     */
>> +    public boolean isShared() {
>> +        return _shared;
>> +    }
>> +
>> +    public void setShared(boolean shared) {
>> +        _shared = shared;
>> +    }
>> +
>> +    /*
>> +     * The classloader that will load any dependancies of the 
>> adapter  or corba skel interfaces.
>> +     * Its should be set by the ejb container
>> +     */
>> +    public ClassLoader getClassLoader() {
>> +        return _cl;
>> +    }
>> +
>> +    public void setClassLoader(ClassLoader cl) {
>> +        _cl = cl;
>> +    }
>
>
> Can we change these to be constructor injected and make the fields  
> final?

MD>> Its possible, I have to discuss this more with Alan .

>
>> +    /*
>> +     * This is the name of the remote class that implements the  
>> remote interface.
>> +     *
>> +     * This is only used if this adapter is going to directly 
>> invoke  an object.  For the
>> +     * EJB Container, the adapter will pass through the method  
>> invocations.
>> +     */
>> +    public String getRemoteClassName() {
>> +        return _remoteClassName;
>> +    }
>> +
>> +    public void setRemoteClassName(String rcName) {
>> +        _remoteClassName = rcName;
>> +    }
>> +
>> +    /*
>> +     * The remote interface name for the remote object.  This will  
>> most likely be the name
>> +     * of the EJB's RemoteInterface and RemoteHomeInterface
>> +     *
>> +     * The stub/skel generator will use this interface name.
>> +     */
>> +    public String getRemoteInterfaceName() {
>> +        return _remoteInterfaceName;
>> +    }
>> +
>> +    public void setRemoteInterfaceName(String riName) {
>> +        _remoteInterfaceName = riName;
>> +    }
>> +
>> +    /*
>> +     * A list of public IDs that the remote object implements:
>> +     *
>> +     * IDL:....:1.0
>> +     * RMI:....:X:Y
>> +     */
>> +    public Vector getIds() {
>> +        return _idVector;
>> +    }
>> +
>> +    public void addId(String id) {
>> +        _idVector.add(id);
>> +    }
>> +
>> +    public void removeId(String id) {
>> +        _idVector.remove(id);
>> +    }
>> +
>> +    /*
>> +     * Return the skeleton implemention for the remote interface.   
>> This interface has the
>> +     * invoke method to handle the rmi/iiop messages.
>> +     */
>> +    public RemoteInterface getRemoteInterface() {
>> +        if (_ri == null) {
>> +            synchronized (this) {
>> +                String riName = _remoteInterfaceName + "_Skeleton";
>> +                _remoteInterfaceClass = loadClass(riName);
>> +
>> +                try {
>> +                    _ri = (RemoteInterface)  
>> _remoteInterfaceClass.newInstance();
>> +                } catch (InstantiationException e) {
>> +                    e.printStackTrace();  //To change body of catch  
>> statement use File | Settings | File Templates.
>> +                } catch (IllegalAccessException e) {
>> +                    e.printStackTrace();  //To change body of catch  
>> statement use File | Settings | File Templates.
>> +                }
>> +            }
>> +        }
>> +
>> +        return _ri;
>> +    }
>
>
> This is double check locking.  The whole method needs to be  
> synchronized.

MD>> The double check code is wrong and I need to change it.    Does the 
method require sync?  Maybe.  I recently came accross a strategy for 
multiprocessor locking that uses a transient int as a memory barrier.  
Probably best if I don't attempt to explain it.  We can optimize our 
locking.  The template that I came accross is as follows:

    private volatile boolean _evaluated = false;

    private Object _value;

    /**
     ** Sub-classes should override this method to get the object's value
     ** when it is first needed.
     **/
    public abstract Object evaluate();

    public final Object getValue()
    {
        if (_volatileIsEffectiveMemoryBarrier)
        {
            if (! _evaluated)
            {
                synchronized (this)
                {
                    if (! _evaluated)
                    {
                        _value = evaluate();
                        _evaluated = true;
                    }
                }
            }
        }
        else
        {
            synchronized (this)
            {
                if (! _evaluated)
                {
                    _value = evaluate();
                    _evaluated = true;
                }
            }
        }
        return _value;
    }

>
>> +    /*
>> +     * Get an object instance to invoke based on the object key.
>> +     *
>> +     * The objectKey could probably be passed to the EJB container 
>> so  that the
>> +     * container can directly invoke the ejb object as required.
>> +     */
>> +    public Object getInstance(byte[] objectKey) {
>> +        String key = new String(objectKey);
>> +        return getInstance(key);
>> +    }
>> +
>> +    public Object getInstance(String key) {
>> +        Object o = _objects.get(key);
>> +
>> +        if (o == null) {
>> +            o = newInstance(key);
>> +        }
>> +
>> +        return o;
>> +    }
>> +
>> +    public Object newInstance(byte[] objectKey) {
>> +        String key = new String(objectKey);
>> +        return newInstance(key);
>> +    }
>> +
>> +    public Object newInstance(String key) {
>> +        Object o = null;
>> +
>> +        if (_remoteClassClass == null) {
>> +            synchronized (this) {
>> +                _remoteClassClass = loadClass(_remoteClassName);
>> +            }
>> +
>> +            try {
>> +                if (_shared) {
>> +                    synchronized (this) {
>> +                        Method m =  
>> _remoteClassClass.getMethod("getInstance", (Class[]) null);
>> +                        o = m.invoke(_remoteClassClass, (Object[])  
>> null);
>> +
>> +                        if (o != null) {
>> +                            _objects.put(key, o);
>> +                        }
>> +                    }
>> +                } else {
>> +                    o = _remoteClassClass.newInstance();
>> +                    _objects.put(key, o);
>> +                }
>> +            } catch (InstantiationException e) {
>> +                e.printStackTrace();  //To change body of catch  
>> statement use File | Settings | File Templates.
>> +            } catch (IllegalAccessException e) {
>> +                e.printStackTrace();  //To change body of catch  
>> statement use File | Settings | File Templates.
>> +            } catch (NoSuchMethodException e) {
>> +                e.printStackTrace();
>> +            } catch (InvocationTargetException e) {
>> +                e.printStackTrace();
>> +            }
>> +        }
>> +
>> +        return o;
>> +    }
>
>
> Again there is double check locking.  Also, if this is called in the  
> critical path, I suggest we use CGLIB instead of reflection for  
> construction as it is way faster.

MD>>  I can look into CGLIB.

>
>> +    /*
>> +     * Invoke method from the IIOP Message Handler.  The adapter is  
>> bound to the INS name service.
>> +     * When an RMI/IIOP message is processed by the server, the  
>> message handler will perform a lookup
>> +     * on the name service to get the Adapter, then the invocation  
>> will be passed to the adapter
>> +     * The adapter will obtain the object key and then determine  
>> which object instance to pass the
>> +     * invocation to.
>> +     */
>
>
> This comment is missing the double star at the top to signify javadoc.
>
>> +    public void invoke(java.lang.String methodName, byte[] 
>> objectKey,  org.apache.geronimo.interop.rmi.iiop.ObjectInputStream 
>> input,  org.apache.geronimo.interop.rmi.iiop.ObjectOutputStream 
>> output) {
>> +        RemoteInterface skeleton = getRemoteInterface();
>> +        Object instance = getInstance(objectKey);
>> +
>> +        if (instance != null) {
>> +            skeleton.$invoke(methodName, objectKey, instance, 
>> input,  output);
>> +        } else {
>> +            throw new org.omg.CORBA.OBJECT_NOT_EXIST(new  
>> String(objectKey));
>> +        }
>> +    }
>> +
>> +    /*
>> +     * Helper function to load a class.  This uses classloader for  
>> the adapter.
>> +     */
>> +    protected Class loadClass(String name) {
>> +        Class c = null;
>> +
>> +        try {
>> +            c = _cl.loadClass(name);
>> +        } catch (Exception e) {
>> +            e.printStackTrace();
>> +        }
>> +
>> +        return c;
>> +    }
>> +}
>
>
> I think we should be at least logging the error or actually letting 
> the  ClassNotFoundException through.  We also have a ClassLoading 
> class  which you may want to use as it handles some of the more weird 
> class  name formats.

MD >> Will change with better Geronimo integration.

>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> adapter/AdapterManager.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/adapter/AdapterManager.java? 
>> view=auto&rev=126264
>> ======================================================================= 
>> =======
>> --- (empty file)
>> +++  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> adapter/AdapterManager.java    Sun Jan 23 23:33:10 2005
>> @@ -0,0 +1,43 @@
>> +/**
>> + *
>> + *  Copyright 2004-2005 The Apache Software Foundation
>> + *
>> + *  Licensed under the Apache License, Version 2.0 (the "License");
>> + *  you may not use this file except in compliance with the License.
>> + *  You may obtain a copy of the License at
>> + *
>> + *     http://www.apache.org/licenses/LICENSE-2.0
>> + *
>> + *  Unless required by applicable law or agreed to in writing,  
>> software
>> + *  distributed under the License is distributed on an "AS IS" BASIS,
>> + *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or  
>> implied.
>> + *
>> + *  See the License for the specific language governing permissions  
>> and
>> + *  limitations under the License.
>> + */
>> +package org.apache.geronimo.interop.adapter;
>> +
>> +import java.util.Hashtable;
>> +
>> +
>> +public class AdapterManager {
>> +    protected Hashtable _adapters;
>> +    protected static AdapterManager _me = new AdapterManager();
>
>
> Underscores and protected...

MD>> Will do.

>
>> +    protected AdapterManager() {
>> +        _adapters = new Hashtable();
>> +    }
>> +
>> +    public static AdapterManager getInstance() {
>> +        return _me;
>> +    }
>
>
> This is a bad idea.  Instead we should have a manger gbean and the  
> singletonness would be handled by there being only one GBean instance  
> registered.

MD>> Sure, this will change with the introduction of GBeans, better 
integration...

>
>> +    public void registerAdapter(Adapter a) {
>> +
>> +        _adapters.put(a.getBindName(), a);
>> +    }
>> +
>> +    public Adapter getAdapter(String objectName) {
>> +        return (Adapter) _adapters.get(objectName);
>> +    }
>
>
> I prefer HashMap and synchronizing the methods that access the Map, 
> but  this works in this case.

MD>> Hashtables are going to be dead soon.  ;-)

>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> client/InitialContextFactory.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/client/InitialContextFactory.java? 
>> view=auto&rev=126264
>> ======================================================================= 
>> =======
>> +public class InitialContextFactory
>> +        implements javax.naming.spi.InitialContextFactory {
>> +    private HashMap _startMap = new HashMap();
>> +
>> +    public Context getInitialContext(java.util.Hashtable env) 
>> throws  NamingException {
>> +        return  
>> org.apache.geronimo.interop.rmi.iiop.client.ClientNamingContext.getInst 
>> ance(env);
>> +    }
>> +}
>
>
> Is the _startMap variable used anywhere?

MD>> I'd like to remove any naming in the interop and just use the 
naming provided in Geronimo.

>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> client/InitialContextFactoryBuilder.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/client/InitialContextFactoryBuilder.java? 
>> view=auto&rev=126264
>> ======================================================================= 
>> =======
>> +public class InitialContextFactoryBuilder
>> +        implements javax.naming.spi.InitialContextFactoryBuilder {
>> +    public javax.naming.spi.InitialContextFactory  
>> createInitialContextFactory(Hashtable env) {
>> +        return new InitialContextFactory();
>> +    }
>> +}
>
>
> What is this for?  Just curious...
>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> generator/GenException.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/generator/GenException.java? 
>> view=auto&rev=126264
>> ======================================================================= 
>> =======
>> +/**
>> + * User: Mark
>> + * Date: Dec 21, 2004
>> + * Time: 3:49:45 PM
>> + */
>
>
> Oops :)
>
>> +public class GenException
>> +        extends Exception {
>> +    public GenException() {
>> +        super();
>> +    }
>> +
>> +    public GenException(String message) {
>> +        super(message);
>> +    }
>> +
>> +    public GenException(Throwable cause) {
>> +        super(cause);
>> +    }
>> +
>> +    public GenException(String message, Throwable cause) {
>> +        super(message, cause);
>> +    }
>> +}
>>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> generator/GenOptions.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/generator/GenOptions.java? 
>> view=auto&rev=126264
>> ======================================================================= 
>> =======
>> +public class GenOptions {
>> +    protected String _genDir = "./";
>> +    protected boolean _overwrite = false;
>> +    protected boolean _verbose = false;
>> +
>> +    public GenOptions() {
>> +    }
>> +
>> +    public GenOptions(String genDir, boolean overwrite, boolean  
>> verbose) {
>> +        _genDir = genDir;
>> +        _overwrite = overwrite;
>> +        _verbose = verbose;
>> +    }
>> +
>> +    public String getGenDir() {
>> +        return _genDir;
>> +    }
>> +
>> +    public void setGenDir(String genDir) {
>> +        _genDir = genDir;
>> +    }
>> +
>> +    public boolean isOverwrite() {
>> +        return _overwrite;
>> +    }
>> +
>> +    public void setOverwrite(boolean overwrite) {
>> +        _overwrite = overwrite;
>> +    }
>> +
>> +    public boolean isVerbose() {
>> +        return _verbose;
>> +    }
>> +
>> +    public void setVerbose(boolean verbose) {
>> +        _verbose = verbose;
>> +    }
>> +
>> +}
>
>
> Do we need both the constructor args and the setters?  If I set a  
> variable is whatever is using this class going to adapt to the 
> recently  changed variable?
>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> naming/NameService.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/naming/NameService.java? 
>> view=auto&rev=126264
>> ======================================================================= 
>> =======
>> --- (empty file)
>> +++  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> naming/NameService.java    Sun Jan 23 23:33:10 2005
>> @@ -0,0 +1,66 @@
>> +/**
>> + *
>> + *  Copyright 2004-2005 The Apache Software Foundation
>> + *
>> + *  Licensed under the Apache License, Version 2.0 (the "License");
>> + *  you may not use this file except in compliance with the License.
>> + *  You may obtain a copy of the License at
>> + *
>> + *     http://www.apache.org/licenses/LICENSE-2.0
>> + *
>> + *  Unless required by applicable law or agreed to in writing,  
>> software
>> + *  distributed under the License is distributed on an "AS IS" BASIS,
>> + *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or  
>> implied.
>> + *
>> + *  See the License for the specific language governing permissions  
>> and
>> + *  limitations under the License.
>> + */
>> +package org.apache.geronimo.interop.naming;
>> +
>> +import java.util.HashMap;
>> +import javax.naming.NamingException;
>> +
>> +import org.apache.geronimo.interop.adapter.Adapter;
>> +
>> +
>> +public class NameService {
>> +    protected static NameService _ns = null;
>
>
> protected and underscore
>
>> +    public static NameService getInstance() {
>> +        if (_ns == null) {
>> +            synchronized (NameService.class) {
>> +                if (_ns == null) {
>> +                    _ns = new NameService();
>> +                    _ns.init();
>> +                }
>> +            }
>> +        }
>> +
>> +        return _ns;
>> +    }
>
>
> Double check locking
>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> naming/NamingContext.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/naming/NamingContext.java? 
>> view=auto&rev=126264
>> ======================================================================= 
>> =======
>> --- (empty file)
>> +++  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> naming/NamingContext.java    Sun Jan 23 23:33:10 2005
>> @@ -0,0 +1,150 @@
>> +/**
>> + *
>> + *  Copyright 2004-2005 The Apache Software Foundation
>> + *
>> + *  Licensed under the Apache License, Version 2.0 (the "License");
>> + *  you may not use this file except in compliance with the License.
>> + *  You may obtain a copy of the License at
>> + *
>> + *     http://www.apache.org/licenses/LICENSE-2.0
>> + *
>> + *  Unless required by applicable law or agreed to in writing,  
>> software
>> + *  distributed under the License is distributed on an "AS IS" BASIS,
>> + *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or  
>> implied.
>> + *
>> + *  See the License for the specific language governing permissions  
>> and
>> + *  limitations under the License.
>> + */
>> +package org.apache.geronimo.interop.naming;
>> +
>> +import java.util.HashMap;
>> +import javax.naming.NameNotFoundException;
>> +import javax.naming.NamingException;
>> +
>> +import org.apache.geronimo.interop.adapter.Adapter;
>> +
>> +
>> +public class NamingContext {
>> +    public static final NamingContext getInstance(Class baseClass) {
>> +        NamingContext context;
>> +        synchronized (_contextMap) {
>> +            context = (NamingContext) _contextMap.get(baseClass);
>> +            if (context == null) {
>> +                context = new NamingContext();
>> +                _contextMap.put(baseClass, context);
>> +                context.init(baseClass);
>> +            }
>> +        }
>> +        return context;
>> +    }
>> +
>> +    private static ThreadLocal _current = new ThreadLocal();
>
>
> Should this be an InheritableThreadLocal?
>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> properties/BooleanProperty.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/properties/BooleanProperty.java? 
>> view=auto&rev=126264
>>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> properties/ByteProperty.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/properties/ByteProperty.java? 
>> view=auto&rev=126264
>>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> properties/DoubleProperty.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/properties/DoubleProperty.java? 
>> view=auto&rev=126264
>>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> properties/FloatProperty.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/properties/FloatProperty.java? 
>> view=auto&rev=126264
>>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> properties/IntProperty.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/properties/IntProperty.java? 
>> view=auto&rev=126264
>>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> properties/LongProperty.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/properties/LongProperty.java? 
>> view=auto&rev=126264
>>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> properties/ShortProperty.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/properties/ShortProperty.java? 
>> view=auto&rev=126264
>>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> properties/StringProperty.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/properties/StringProperty.java? 
>> view=auto&rev=126264
>
>
> How are these used?
>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> properties/SystemProperties.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/properties/SystemProperties.java? 
>> view=auto&rev=126264
>
>
> Is this a bridge to java.lang.System properties?  If so, do we really  
> want to support system properties?
>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> repository/Repository.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/repository/Repository.java? 
>> view=auto&rev=126264
>> ======================================================================= 
>> =======
>> +public class Repository {
>> +    // ??
>> +}
>
>
> Say what? :)
>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> rmi/iiop/IiopVersion.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/rmi/iiop/IiopVersion.java? 
>> view=auto&rev=126264
>> ======================================================================= 
>> =======
>> +public class IiopVersion {
>> +}
>
>
> What's this for?
>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> rmi/iiop/ListenerInfo.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/rmi/iiop/ListenerInfo.java? 
>> view=auto&rev=126264
>> +public class ListenerInfo {
>> +    public int protocol;
>> +
>> +    public String host;
>> +
>> +    public int port;
>> +}
>
>
> Do these have to be public fields?  Normally we use, private fields  
> with getters and setters.
>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> rmi/iiop/NameServiceOperations_Skeleton.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/rmi/iiop/ 
>> NameServiceOperations_Skeleton.java?view=auto&rev=126264
>
>
> Do we want the Skeletons checked in?
>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> rmi/iiop/ObjectKey.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/rmi/iiop/ObjectKey.java? 
>> view=auto&rev=126264
>> ======================================================================= 
>> =======
>> +public class ObjectKey {
>> +    public static final int TYPE_MANAGER = 'M';
>> +
>> +    public static final int TYPE_SESSION = 'S';
>> +
>> +    public int type;
>> +    public String username = "";
>> +    public String password = "";
>> +    public String component = "";
>> +    public String sessionID = "";
>
>
> public fields?
>
>> +    public void checkPassword() {
>> +        User.getInstance(username).login(password);
>> +    }
>
>
> Shouldn't this go though Geronimo security?
>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> rmi/iiop/ObjectRef.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/rmi/iiop/ObjectRef.java? 
>> view=auto&rev=126264
>> ======================================================================= 
>> =======
>> +    public int $getIiopVersion() {
>> +        return _iiopVersion;
>> +    }
>
>
> Why do all the methods in this class start with a dollar sign?
>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> rmi/iiop/client/ClientNamingContext.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/rmi/iiop/client/ClientNamingContext.java? 
>> view=auto&rev=126264
>
>
> We should consider merging this with the Geronimo naming stuff... not 
> a  task for today, but (much) later on :)
>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> rmi/iiop/compiler/StubCompiler.txt
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/rmi/iiop/compiler/StubCompiler.txt? 
>> view=auto&rev=126264
>
>
> Why is this a text file?
>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> rmi/iiop/server/SocketListener.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/rmi/iiop/server/SocketListener.java? 
>> view=auto&rev=126264
>> ======================================================================= 
>> =======
>> +public class SocketListener extends Thread {
>> +    public static SocketListener getInstance() {
>> +        return new SocketListener();
>> +    }
>
>
> Why is there a static factory?
>
>> +    // private data
>> +
>> +    private String _name;
>> +
>> +    private String _host;
>> +
>> +    private int _port;
>> +
>> +    private int _listenBacklog;
>> +
>> +    private java.net.ServerSocket _listener;
>> +
>> +    // internal methods
>> +
>> +    protected void init() {
>> +        _host = "localhost";
>> +        _port = 2000;
>> +        _listenBacklog = 10;
>> +        setDaemon(true);
>> +    }
>> +
>> +    // public methods
>> +
>> +    public void setHost(String host) {
>> +        _host = host;
>> +    }
>> +
>> +    public void setPort(int port) {
>> +        _port = port;
>> +    }
>> +
>> +    public void setListenBacklog(int backlog) {
>> +        _listenBacklog = backlog;
>> +    }
>> +
>> +    public void run() {
>> +        String iiopURL = "iiop://" + _host + ":" + _port;
>> +        ListenerInfo listenerInfo = new ListenerInfo();
>> +        listenerInfo.protocol = Protocol.IIOP; // TODO: other  
>> protocols (IIOPS etc.)
>> +        listenerInfo.host = _host;
>> +        listenerInfo.port = _port;
>> +        try {
>> +            InetAddress addr = InetAddress.getByName(_host);
>> +            _listener = new java.net.ServerSocket(_port,  
>> _listenBacklog, addr);
>> +        } catch (Exception ex) {
>> +            System.out.println("SocketListener: Error creating 
>> server  socket.");
>> +            ex.printStackTrace();
>> +            try {
>> +                Socket socket = new Socket(_host, _port);
>> +                socket.close();
>> +                System.out.println("SocketListener: Error server  
>> already running: " + iiopURL);
>> +                ex.printStackTrace();
>> +            } catch (Exception ignore) {
>> +            }
>> +            return;
>> +        }
>> +        new CheckConnect().start();
>> +        for (; ;) {
>> +            java.net.Socket socket;
>> +            try {
>> +                socket = _listener.accept();
>> +            } catch (Exception ex) {
>> +                throw new SystemException(ex); // TODO: log error  
>> message
>> +            }
>> +            MessageHandler.getInstance(listenerInfo, socket).start();
>> +        }
>> +    }
>> +
>> +    private class CheckConnect extends Thread {
>> +        public void run() {
>> +            try {
>> +                Socket socket = new Socket(_host, _port);
>> +                socket.close();
>> +                if (!SystemProperties.quiet()) {
>> +                     
>> System.out.println(formatAcceptingIiopConnections());
>> +                }
>> +            } catch (Exception ex) {
>> +                warnConnectFailed(_host, _port);
>> +            }
>> +        }
>> +    }
>> +
>> +    // format methods
>> +
>> +    protected String formatAcceptingIiopConnections() {
>> +        String msg = 
>> "SocketListener.formatAcceptingIiopConnection():  ";
>> +        return msg;
>> +    }
>> +
>> +    // log methods
>> +
>> +    protected void warnConnectFailed(String host, int port) {
>> +        System.out.println("SocketListener.warnConnectFailed(): 
>> host  = " + host + ", port = " + port);
>> +    }
>> +}
>
>
> I suggest, you either require all state data such as host and port be  
> passed in via the constructor, or make the setters throw an "already  
> running" exception.  Otherwise people mistakenly think the can change  
> the port.
>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> security/Role.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/security/Role.java?view=auto&rev=126264
>>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> security/SimpleSubject.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/security/SimpleSubject.java? 
>> view=auto&rev=126264
>>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> security/User.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/security/User.java?view=auto&rev=126264
>
>
> What is the plan for these with regards to the Geronimo security module?
>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> util/SystemUtil.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/util/SystemUtil.java?view=auto&rev=126264
>> ======================================================================= 
>> =======
>> +public class SystemUtil {
>> +    // properties
>> +
>> +    public static final StringProperty vmVersionProperty =
>> +            new StringProperty(SystemProperties.class,  
>> "java.vm.version");
>> +
>> +    // private data
>> +
>> +    private static String _vmVersion = vmVersionProperty.getString();
>> +
>> +    private static boolean _isJDK13 = _vmVersion.startsWith("1.3")
>> +                                      ||  
>> _vmVersion.startsWith("CrE-ME V4.00");
>> +
>> +    private static boolean _isJDK14 = _vmVersion.startsWith("1.4");
>> +
>> +    // public methods
>> +
>> +    public static String getExecutableSuffix() {
>> +        return isWindows() ? ".exe" : "";
>> +    }
>> +
>> +    public static String getShellScriptSuffix() {
>> +        return isWindows() ? ".bat" : ".sh";
>> +    }
>> +
>> +    public static boolean isJDK13() {
>> +        return _isJDK13;
>> +    }
>> +
>> +    public static boolean isJDK14() {
>> +        return _isJDK14;
>> +    }
>> +
>> +    public static boolean isWindows() {
>> +        return java.io.File.separatorChar == '\\';
>> +    }
>> +}
>
>
> How is this used?
>
>> Added:  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> util/ThreadContext.java
>> Url:  
>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
>> org/apache/geronimo/interop/util/ThreadContext.java? 
>> view=auto&rev=126264
>> ======================================================================= 
>> =======
>> --- (empty file)
>> +++  
>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
>> util/ThreadContext.java    Sun Jan 23 23:33:10 2005
>> @@ -0,0 +1,135 @@
>> +/**
>> + *
>> + *  Copyright 2004-2005 The Apache Software Foundation
>> + *
>> + *  Licensed under the Apache License, Version 2.0 (the "License");
>> + *  you may not use this file except in compliance with the License.
>> + *  You may obtain a copy of the License at
>> + *
>> + *     http://www.apache.org/licenses/LICENSE-2.0
>> + *
>> + *  Unless required by applicable law or agreed to in writing,  
>> software
>> + *  distributed under the License is distributed on an "AS IS" BASIS,
>> + *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or  
>> implied.
>> + *
>> + *  See the License for the specific language governing permissions  
>> and
>> + *  limitations under the License.
>> + */
>> +package org.apache.geronimo.interop.util;
>> +
>> +import java.util.HashMap;
>> +
>> +import org.apache.geronimo.interop.SystemException;
>> +
>> +
>> +public abstract class ThreadContext {
>> +    private static HashMap _primTypes;
>> +
>> +    private static ThreadLocal _defaultRmiHost = new ThreadLocal();
>> +
>> +    private static ThreadLocal _defaultRmiPort = new ThreadLocal();
>> +
>> +    static {
>> +        _primTypes = new HashMap();
>> +        _primTypes.put("boolean", boolean.class);
>> +        _primTypes.put("char", char.class);
>> +        _primTypes.put("byte", byte.class);
>> +        _primTypes.put("short", short.class);
>> +        _primTypes.put("int", int.class);
>> +        _primTypes.put("long", long.class);
>> +        _primTypes.put("float", float.class);
>> +        _primTypes.put("double", double.class);
>> +        _primTypes.put("boolean[]", boolean[].class);
>> +        _primTypes.put("char[]", char[].class);
>> +        _primTypes.put("byte[]", byte[].class);
>> +        _primTypes.put("short[]", short[].class);
>> +        _primTypes.put("int[]", int[].class);
>> +        _primTypes.put("long[]", long[].class);
>> +        _primTypes.put("float[]", float[].class);
>> +        _primTypes.put("double[]", double[].class);
>> +    }
>> +
>> +    public static String getDefaultRmiHost() {
>> +        String host = (String) _defaultRmiHost.get();
>> +        if (host == null) {
>> +            host = "0";
>> +        }
>> +        return host;
>> +    }
>> +
>> +    public static int getDefaultRmiPort() {
>> +        Integer port = (Integer) _defaultRmiPort.get();
>> +        if (port == null) {
>> +            port = IntegerCache.get(0);
>> +        }
>> +        return port.intValue();
>> +    }
>> +
>> +    public static Class loadClass(String className) {
>> +        Class t = (Class) _primTypes.get(className);
>> +        if (t != null) {
>> +            return t;
>> +        }
>> +        try {
>> +            ClassLoader loader =  
>> Thread.currentThread().getContextClassLoader();
>> +            if (loader == null) {
>> +                return Class.forName(className);
>> +            } else {
>> +                return loader.loadClass(className);
>> +            }
>> +        } catch (RuntimeException ex) {
>> +            throw (RuntimeException) ex;
>> +        } catch (Exception ex) {
>> +            throw new SystemException(ex);
>> +        }
>> +    }
>
>
> This is unlikely to work in Geronimo as we almost never set context  
> class loader.  Normally we try to explicitly pass around the  
> classloader.
>
>> +
>> +    public static Class loadClass(String className, Class  
>> parentClass) {
>> +        if (parentClass == null) {
>> +            return loadClass(className);
>> +        }
>> +        Class t = (Class) _primTypes.get(className);
>> +        if (t != null) {
>> +            return t;
>> +        }
>> +        try {
>> +            ClassLoader loader = parentClass.getClassLoader();
>> +            if (loader == null) {
>> +                return loadClass(className);
>> +            } else {
>> +                return loader.loadClass(className);
>> +            }
>> +        } catch (RuntimeException ex) {
>> +            throw (RuntimeException) ex;
>> +        } catch (Exception ex) {
>> +            throw new SystemException(ex);
>> +        }
>> +    }
>> +
>> +    public static Class loadClassOrReturnNullIfNotFound(String  
>> className) {
>> +        try {
>> +            return loadClass(className);
>> +        } catch (RuntimeException ex) {
>> +            return null;
>> +        }
>> +    }
>> +
>> +    public static Class loadClassOrReturnNullIfNotFound(String  
>> className, Class parentClass) {
>> +        if (parentClass == null) {
>> +            return loadClassOrReturnNullIfNotFound(className);
>> +        }
>> +        try {
>> +            return loadClass(className, parentClass);
>> +        } catch (RuntimeException ex) {
>> +            return null;
>> +        }
>> +    }
>
>
> Most of this class loader stuff is handled by the class ClassLoading  
> which handles some of the more weird stuff.
>
>
> .
>


Mime
View raw message