river-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Trasuk <tras...@stratuscom.com>
Subject Re: Don't let Jini Standards become an impediment to development
Date Wed, 09 Sep 2015 03:40:30 GMT

> On Sep 8, 2015, at 10:40 PM, Peter <jini@zeus.net.au> wrote:
> 
> On 8/09/2015 11:26 PM, Greg Trasuk wrote:
>> That’s the current state.  Changing (e.g. by enforcing a builder pattern or something)
would add unneeded complexity for the user if you ask me.
> 
> Yes, I agree, a constructor is suitable, keep it simple.
> 
>> In fact, the fact that the Entry fields are non-final is the way Jini knows it’s
a field and not a constant.  i.e. in the AbstractEntry docs, it specifically says "The entry
fields of an Entry are its public, non-primitive, non-static, non-transient, non-final fields."
> 
> There's no code that inspects the field and checks whether it's final, not within River
that I'm aware of, feel free to show me where if I'm wrong :).

com.sun.jini.outrigger.EntryRep.  Look for ‘Modifier.FINAL’ in the ‘usableField(…)’
method.  There are 12 other uses of ‘FINAL’ in River 2.2.2.  Not to mention any external
product (Blitz, Rio, who knows what) that are designed around the Entry specification.

The EntryRep you posted below is com.sun.jini.reggie.EntryRep.  You’ll notice that it calls
com.sun.jini.reggie.ClassMapper, which also filters final fields out of the comparisons and
mappings.

>  The reason these fields are non final, is so they can be set with reflection, this is
a detail of the Entry specification we should look at changing.
> 

The reason they’re non-final is so that services like Reggie and Outrigger know what constitutes
an Entry field that they might need to match.  Entry fields are used to form the templates
for matching entries.

> Entry's are not subject to the usual serialization rules.  All fields in an Entry in
superclass to subclass order are stored in an EntryRep (appended).  
> At present these are set after construction using reflection, however a constructor that
accepts an array parameter, will allow the child most class to pass that array up through
all constructors to reconstruct the Entry without using reflection (magnitudes faster performance
wise).
> 
> Eliminating the use of reflection during deserialization will increase performance, and
immutability increases scalability.
> 
>> Where do we use entries?  Typically in doing Service registrations and lookups. 
Most users can understand that the Entry is going to be serialized and used remotely.
> Clients of Javaspaces and utility classes too, like JoinManager and ServiceDiscoveryManager.
>> Add to documentation?  Sure.  Change the API?  Maybe add an optional builder-style
object.  (e.g. create a StatusBuilder that acts as a bean and generates Status entries). 
But even so, it seems like a small enough part of using Jini to not bother much with.
> 
> Due to the way the current Entry spec works, you can't add a field to an Entry without
breaking compatibility with subclassses.
> 
> So the proposed change would be a new public constructor, that accepts an array, containing
field object values in the order that fields occur and that all fields be final.
> 
> I'd also propose including a warning that once published the order and number of fields
in an Entry should not be changed if it can be extended, otherwise if there's a chance that
additional fields might need to be appended at a later date, a reccommendation that the class
be made final, so it can't be subclassed.
> 
> So in other words, the following changes will break an Entry:
> 
>  1. A change in the class heirarchy.
>  2. A change in the order or number or type of fields.
> 
> An Entry is best thought of an interface definition for a defined group of objects in
serial form.
> 
>> 
>> 
>> Another way to think about it is that Entries don’t mean much until you send them
somewhere.  So you need to make sure they’re all setup and everything they refer to is stable
when you send them.  To me, that’s a user-understanding item that’s difficult to enforce
through code.
> 
> But because Entry fields are mutable and unsynchronized, and used in utility classes
such as ServiceDiscoveryManager, it becomes difficult to manage when Entry's are passed around
between threads.   To submit an Entry to a service, it is passed to a thread pool where it
becomes a serialized method invocation.
> 
> To ensure the changes made by one thread are visible to another, the Entry must be published
safely, otherwise the changes can't be guaranteed visible between threads.  There is no synchronization
on Entry field access, so this makes it very difficult to reason about an Entry, one strategy
I've adopted is defensive copying, however all this does, is guarantee that any modifications
made by client code, doesn't affect a copy in ServiceDiscoveryManager (for example), there
is a memory usage cost, because internal copies can't be shared with client ServiceItemFilter’s

Typical usage of a ServiceDiscoveryManager, for instance, would be if I want to perform a
lookup I’ll create a ServiceTemplate that contains the Entries I want matched.  If I want
to create a LookupCache, I’ll pass in a ServiceTemplate to the createLookupCache method.

If you have a separate thread that messes around with those entries while the lookup is in
process, or while the LookupCache is active, that’s a client error - you deserve what you
get.

If ServiceDiscoveryManager does it wrong, we need to fix ServiceDiscoveryManager.

> .
> 

> Reasoning about shared state becomes much simpler if Entry's are immutable and the upside
is scalability and performance improves, especially for clients of lookup services deserializing
a lot of Entry objects.
> 

Entries aren’t intended to encapsulate shared state in any widely concurrent way.  They’re
intended to be created on a thread, and then sent over the network.  If there is shared state
inside a given service, that’s the concern of the service implementation.  Does Outrigger
handle its concurrency correctly?  I don’t know- I’ve never looked.  If it doesn’t then
we need to fix Outrigger, not change the Entry specification.

It’s good that we’re having these discussions, so we can make sure that the real-world
usages get taken into account before we go altering the specifications.

Cheers,

Greg Trasuk.


> /**
> * An EntryRep contains the fields of an Entry packaged up for
> * transmission between client-side proxies and the registrar server.
> * Instances are never visible to clients, they are private to the
> * communication between the proxies and the server.
> * <p>
> * This class only has a bare minimum of methods, to minimize
> * the amount of code downloaded into clients.
> *
> * @author Sun Microsystems, Inc.
> *
> */
> class EntryRep implements Serializable, Cloneable {
> 
>    private static final long serialVersionUID = 2L;
> 
>    /**
>     * The Class of the Entry converted to EntryClass.
>     *
>     * @serial
>     */
>    public EntryClass eclass;
>    /**
>     * The codebase of the entry class.
>     *
>     * @serial
>     */
>    public String codebase;
>    /**
>     * The public fields of the Entry, each converted as necessary to
>     * a MarshalledWrapper (or left as is if of known java.lang immutable
>     * type).  The fields are in super- to subclass order.
>     *
>     * @serial
>     */
>    public Object[] fields;
> 
>    /**
>     * Converts an Entry to an EntryRep.  Any exception that results
>     * is bundled up into a MarshalException.
>     */
>    public EntryRep(Entry entry) throws RemoteException {
>    EntryClassBase ecb = ClassMapper.toEntryClassBase(entry.getClass());
>    eclass = ecb.eclass;
>    codebase = ecb.codebase;
>    try {
>        EntryField[] efields = ClassMapper.getFields(entry.getClass());
>        fields = new Object[efields.length];
>        for (int i = efields.length; --i >= 0; ) {
>        EntryField f = efields[i];
>        Object val = f.field.get(entry);
>        if (f.marshal && val != null)
>            val = new MarshalledWrapper(val);
>        fields[i] = val;
>        }
>    } catch (IOException e) {
>        throw new MarshalException("error marshalling arguments", e);
>    } catch (IllegalAccessException e) {
>        throw new MarshalException("error marshalling arguments", e);
>    }
>    }
> 
>    /**
>     * Convert back to an Entry.  If the Entry cannot be constructed,
>     * null is returned.  If a field cannot be unmarshalled, it is set
>     * to null.
>     */
>    public Entry get() {
>    try {
>        Class clazz = eclass.toClass(codebase);
>        EntryField[] efields = ClassMapper.getFields(clazz);
>        Entry entry = (Entry)clazz.newInstance();
>        for (int i = efields.length; --i >= 0; ) {
>        Object val = fields[i];
>        EntryField f = efields[i];
>        Field rf = f.field;
>        try {
>            if (f.marshal && val != null)
>            val = ((MarshalledWrapper) val).get();
>            rf.set(entry, val);
>        } catch (Throwable e) {
>            if (e instanceof IllegalArgumentException) {
>            // fix 4872566: work around empty exception message
>            String msg = "unable to assign " +
>                ((val != null) ?
>                "value of type " + val.getClass().getName() :
>                "null") +
>                " to field " + rf.getDeclaringClass().getName() +
>                "." + rf.getName() + " of type " +
>                rf.getType().getName();
>            e = new ClassCastException(msg).initCause(e);
>            }
>            RegistrarProxy.handleException(e);
>        }
>        }
>        return entry;
>    } catch (Throwable e) {
>        RegistrarProxy.handleException(e);
>    }
>    return null;
>    }
> 
>    /**
>     * We don't need this in the client or the server, but since we
>     * redefine equals we provide a minimal hashCode that works.
>     */
>    public int hashCode() {
>    return eclass.hashCode();
>    }
> 
>    /**
>     * EntryReps are equal if they have the same class and the fields
>     * are pairwise equal.  This is really only needed in the server,
>     * but it's very convenient to have here.
>     */
>    public boolean equals(Object obj) {
>    if (obj instanceof EntryRep) {
>        EntryRep entry = (EntryRep)obj;
>        if (!eclass.equals(entry.eclass) ||
>        fields.length != entry.fields.length)
>        return false;
>        for (int i = fields.length; --i >= 0; ) {
>        if ((fields[i] == null && entry.fields[i] != null) ||
>            (fields[i] != null && !fields[i].equals(entry.fields[i])))
>            return false;
>        }
>        return true;
>    }
>    return false;
>    }
> 
>    /**
>     * Deep clone (which just means cloning the fields array too).
>     * This is really only needed in the server, but it's very
>     * convenient to have here.
>     */
>    public Object clone() {
>    try {
>        EntryRep entry = (EntryRep)super.clone();
>        entry.fields = (Object[])entry.fields.clone();
>        return entry;
>    } catch (CloneNotSupportedException e) {
>        throw new InternalError();
>    }
>    }
> 
>    /**
>     * Converts an array of Entry to an array of EntryRep.  If needCodebase
>     * is false, then the codebase of every EntryRep will be null.
>     */
>    public static EntryRep[] toEntryRep(Entry[] entries, boolean needCodebase)
>    throws RemoteException
>    {
>    EntryRep[] reps = null;
>    if (entries != null) {
>        reps = new EntryRep[entries.length];
>        for (int i = entries.length; --i >= 0; ) {
>        if (entries[i] != null) {
>            reps[i] = new EntryRep(entries[i]);
>            if (!needCodebase)
>            reps[i].codebase = null;
>        }
>        }
>    }
>    return reps;
>    }
> 
>    /** Converts an array of EntryRep to an array of Entry. */
>    public static Entry[] toEntry(EntryRep[] reps) {
>    Entry[] entries = null;
>    if (reps != null) {
>        entries = new Entry[reps.length];
>        for (int i = reps.length; --i >= 0; ) {
>        entries[i] = reps[i].get();
>        }
>    }
>    return entries;
>    }
> }
> 


Mime
View raw message