You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geronimo.apache.org by Dain Sundstrom <ds...@gluecode.com> on 2005/01/24 23:53:39 UTC
Review - svn commit: r126264
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.
> 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.
> 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.
> 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.
> 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.
> + 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.
> + //
> + // 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?
> + /*
> + * 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.
> + /*
> + * 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.
> + /*
> + * 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.
> 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...
> + 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.
> + 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.
> 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?
> 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.
Re: Review - svn commit: r126264
Posted by Dain Sundstrom <ds...@gluecode.com>.
On Mar 14, 2005, at 7:07 AM, Mark wrote:
> Many appologies - I just haven't had the time to clean up the code on
> my laptop with all my changes and then get them submitted for a commit
> by somebody else. I have a few outstanding items to resolve before
> submitting the code to check in. I know that you have been busy
> working towards CTS certification and since the interop code won't be
> used for the certification, I have lowered the priority on my code.
> :-( However, ...
>
> Yes, my code properly handles the method name overload as per the
> RMI-IIOP spec.
>
> Hopefully within the next few days, I will have a new zip file with
> all my changes so they can be committed.
Can you create a diff instead of a zip? Diffs are much easier to deal
with.
My guess is you already know this, but I'll go over it again for the
lurkers on the list. Creating a diff with subversion is very easy.
You can add and remove files with 'svn add' and 'svn remove'
respectively. Then just run 'svn diff' from the root directory, and
pipe the result to a file. Post this in a JIRA issue, and one of the
committers will look it over.
-dain
Re: Review - svn commit: r126264
Posted by Mark <de...@hotmail.com>.
Many appologies - I just haven't had the time to clean up the code on my
laptop with all my changes and then get them submitted for a commit by
somebody else. I have a few outstanding items to resolve before
submitting the code to check in. I know that you have been busy working
towards CTS certification and since the interop code won't be used for
the certification, I have lowered the priority on my code. :-( However, ...
Yes, my code properly handles the method name overload as per the
RMI-IIOP spec.
Hopefully within the next few days, I will have a new zip file with all
my changes so they can be committed.
Thanks
Mark
David Jencks wrote:
> I started looking through this also -- it looks very nice. However,
> I have an ulterior motive :-) If I understand the stub/skeleton
> generation code correctly, it does not currently handle overloaded
> methods properly, it just assumes the IDL operation name is the same
> as the java method name. Do you have just sitting there waiting to
> submit or are you planning to write soon code to translate java names
> to IDL operation names taking account of overloaded methods?
>
> Many thanks,
> david jencks
>
>
> On Jan 24, 2005, at 7:28 PM, Mark wrote:
>
>> 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.getIn
>>>> st 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.
>>>
>>>
>>> .
>>>
>>
>
>
> .
>
Re: Review - svn commit: r126264
Posted by David Jencks <da...@yahoo.com>.
I started looking through this also -- it looks very nice. However, I
have an ulterior motive :-) If I understand the stub/skeleton
generation code correctly, it does not currently handle overloaded
methods properly, it just assumes the IDL operation name is the same as
the java method name. Do you have just sitting there waiting to submit
or are you planning to write soon code to translate java names to IDL
operation names taking account of overloaded methods?
Many thanks,
david jencks
On Jan 24, 2005, at 7:28 PM, Mark wrote:
> 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.getIn
>>> st 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.
>>
>>
>> .
>>
>
Re: Review - svn commit: r126264
Posted by David Jencks <dj...@gluecode.com>.
On Jan 26, 2005, at 7:55 AM, Mark wrote:
> Is it safe to say the we should use CGLIB instead of the
> java.lang.reflect package?
For deployment/startup code it doesn't matter much. For code executed
during a request it is a good idea to use CGLIB.
I'm in favor of enhancing the classes during deployment and including
the enhanced classes in the generated configuration. An example of how
to do this is in the AxisBuilder. Please ask if you have any
questions.
thanks
david jencks
>
> Thanks
> Mark
>
>>>> 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.
>>
>>
>> No rush on that one... it's for the polish phase.
>>
>
Re: Review - svn commit: r126264
Posted by Mark <de...@hotmail.com>.
Is it safe to say the we should use CGLIB instead of the
java.lang.reflect package?
Thanks
Mark
>>> 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.
>
>
> No rush on that one... it's for the polish phase.
>
Re: Review - svn commit: r126264
Posted by Dain Sundstrom <ds...@gluecode.com>.
Finally got some free time to respond again :)
On Jan 24, 2005, at 7:28 PM, Mark wrote:
> 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.
Excellent!
> 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.
Yep. The Geronimo deployment code is similar; not fast or that
beautiful, but works.
> 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. :-)
I just wanted to point out some of the stuff I noticed. I figure the
neat/tight integration will take long time (months) as all of us get
accustomed to new integrated code base.
> 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.
Great... the rest of my comments are inline (I sniped out the stuff
where I don't have any other comments... darn thing is just too long :)
> Dain Sundstrom wrote:
>>> 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
I see. You many want to put a big warning at the top saying "don't
bother messing with this is generated and going away".
>>> 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 think you lost me. What is it? The reason I thought is was test
code was because it had an echo method that looks like test code I
normally write.
> 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.
I personally would like to see less plans in Geronimo. I personally
find all the plans confusing, but others disagree with me :)
>>> + /*
>>> + * 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;
> }
Jeremy and I tried a structure like this when we were building locking
code for geronimo. The problem we found was evaluating a volatile is
like 10 (maybe 100) times slower then entering a synchronized block.
It seems that Sun has worked a ton on optimizing synchronized blocked
and virtually no work on volatiles. Regardless, I'd go for the easier
to read simple synchronize code unless it's on the critical path (and
then I'd do some testing :)
>> 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.
No rush on that one... it's for the polish phase.
>>> 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.getIn
>>> st 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.
I just recently re-read the code in geronimo and the code is almost
identical. I think the only difference is the implementation in
geronimo-naming supports compound name (java:comp/env) lookup from
within a context.
Re: Review - svn commit: r126264
Posted by Mark <de...@hotmail.com>.
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.
>
>
> .
>