Peter have you ever constructed a demo of a massively active Javaspace app with
and without reflection use for entry deserialization? It might be a great
thing to have those numbers to help everyone recognize what you (and I) have
seen as impeding implementation/architectural attributes.
Gregg
> On Sep 8, 2015, at 11:30 PM, Peter <j...@zeus.net.au> wrote:
>
> Thanks Greg,
>
> Was it a case of; because we can't set final fields (well not without a
> Permission anyway), that they shouldn't be included in Entry serialized
> state, because then we can't deserialize them?
>
> I've done my best to fix the existing implementations, so hopefully they
> won't need further fixes, however, the fixes were very difficult and these
> implementations very difficult to reason about, because there is so much
> mutable state. In ServiceDiscoveryManager, a thread holds a lock while
> waiting for the result of a remote call, there was no solution I could find
> to remove this lock.
>
> To quote Keith Edwards "The Special Semantics of Attributes":
>
> "All the methods of the object are ignored for purposes of
> searching, as are "special" data fields: static, transient,
> non-public, or final fields. Likewise all fields that are primitive
> types (such as ints and booleans) are ignored; only references to
> other objects within an attribute are considered for searching."
>
>
> So our choices are (for River 4.0):
>
> 1. Break backward compatibility and increase scalability, performance
> and reduce bugs, by not ignoring final fields in Entry's, but
> instead mandating them.
> 2. Or continue full compatibility and live with lower performance,
> less scalability and harder to debug code.
>
> I think there's plenty of time for implementations to prepare for River 4.0,
> if we start talking about it now.
>
> Regards,
>
> Peter.
>
> How are these for code comments (from ServiceDiscoveryManager)?
>
> // Don't like the fact that we're calling foreign code while
> // holding an object lock, however holding this lock doesn't
> // provide an opportunity for DOS as the lock only relates to
> a specific
> // ServiceRegistrar and doesn't interact with client code.
> matches = proxy.lookup(tmpl, Integer.MAX_VALUE);
>
> /* The cache must be created inside the listener sync block,
>
> * otherwise a race condition can occur. This is because the
> * creation of a cache results in event registration which
> * will ultimately result in the invocation of the serviceAdded()
> * method in the cache's listener, and the interruption of any
> * objects waiting on the cache's listener. If the notifications
> * happen to occur before commencing the wait on the listener
> * object (see below), then the wait will never be interrupted
> * because the interrupts were sent before the wait() method
> * was invoked. Synchronizing on the listener and the listener's
> * serviceAdded() method, and creating the cache only after the
> * lock has been acquired, together will prevent this situation
> * since event registration cannot occur until the cache is
> * created, and the lock that allows entry into the serviceAdded()
> * method (which is invoked once the events do arrive) is not
> * released until the wait() method is invoked .
> */
>
> /**
> * With respect to a given service (referenced by the parameter
> * newItem), if either an event has been received from the given lookup
> * service (referenced by the proxy parameter), or a snapshot of the
> * given lookup service's state has been retrieved, this method
> * determines whether the service's attributes have changed, or whether
> * a new version of the service has been registered. After the
> * appropriate determination has been made, this method applies the
> * filter associated with the current cache and sends the appropriate
> * local ServiceDiscoveryEvent(s).
> *
> * This method is called under the following conditions: - when a new
> * lookup service is discovered, this method will be called for each
> * previously discovered service - when a gap in the events from a
> * previously discovered lookup service is discovered, this method will
> * be called for each previously discovered service - when a
> MATCH_MATCH
> * event is received, this method will be called for each previously
> * discovered service - when a NOMATCH_MATCH event is received, this
> * method will be called for each previously discovered service Note
> * that this method is never called when a MATCH_NOMATCH event is
> * received; such an event is always handled by the handleMatchNoMatch
> * method.
> *
> * When this method is called, it may send one of the following events
> * or combination of events: - a service changed event - a service
> * removed event followed by a service added event - a service removed
> * event
> *
> * A service removed event is sent when the service either fails the
> * filter, or the filter produces an indefinite result; in which case,
> * the service is also discarded.
> *
> * A service changed event is sent when the service passes the filter,
> * and it is determined that the service's attributes have changed. In
> * this case, the old and new service proxies are treated as the same
> if
> * one of the following conditions is met: - this method was called
> * because of the receipt of a MATCH_MATCH event - the old and new
> * service proxies are byte-wise fully equal (Note that the lookup
> * service specification guarantees that the proxies are the same when
> a
> * MATCH_MATCH event is received.)
> *
> * A service removed event followed by a service added event is sent
> * when the service passes the filter, and the conditions for which a
> * service changed event would be considered are not met; that is, this
> * method was not called because of the receipt of a MATCH_MATCH event;
> * or the old and new service proxies are not byte-wise fully equal.
> *
> * The if-else-block contained in this method implements the logic just
> * described. The parameter matchMatchEvent reflects the pertinent
> event
> * state that causes this method to be called. That is, either a
> * MATCH_MATCH event was received, or it wasn't, (and if it wasn't,
> then
> * a full byte-wise comparison is performed to determine whether the
> * proxies are still the same).
> *
> * To understand when the 'else' part of the if-else-block is executed,
> * consider the following conditions: - there is more than one lookup
> * service with which the service registers (ex. LUS-0 and LUS-1) -
> * after the service registers with LUS-0, a NOMATCH_MATCH event is
> * received and handled (so the service is now known to the cache) -
> * before the service registers with LUS-1, the service is replaced
> with
> * a new version - the NOMATCH_MATCH event resulting from the service's
> * registration with LUS-1 is received BEFORE receiving the
> * MATCH_NOMATCH/NOMATCH_MATCH event sequence that will ultimately
> * result from the re-registration of that new version with LUS-0 When
> * the above conditions occur, the NOMATCH_MATCH event that resulted
> * from the service's registration with LUS-1 will cause this method to
> * be invoked and the proxies to be fully compared (because the event
> * was not a MATCH_MATCH event); and since the old service proxy and
> the
> * new service proxy will not be fully equal, the else part of the
> * if-else-block will be executed.
> *
> * This method applies the filter only after the above comparisons and
> * determinations have been completed.
> */
>
> On 9/09/2015 1:40 PM, Greg Trasuk wrote:
>>> On Sep 8, 2015, at 10:40 PM, Peter<j...@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;
>>> }
>>> }
>>>
>>
>
