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, [EMAIL PROTECTED] 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.
.
.
