Mailing-List: contact dev-help@river.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@river.apache.org
Message-ID: <570B0CE7.5020205@zeus.net.au>
Date: Mon, 11 Apr 2016 12:33:11 +1000
From: Peter <jini@zeus.net.au>
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB;
 rv:1.9.2.28) Gecko/20120306 Thunderbird/3.1.20
MIME-Version: 1.0
To: dev@river.apache.org
Subject: Re: svn commit: r1738402 -
 /river/jtsk/trunk/src/net/jini/core/event/RemoteEvent.java
References: <20160410015747.0BE313A00EC@svn01-us-west.apache.org>
 <3501C056-C16E-4EBE-A3B9-FA1CCD525EEC@stratuscom.com>
In-Reply-To: <3501C056-C16E-4EBE-A3B9-FA1CCD525EEC@stratuscom.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


First, please consider the following example:

It might be easier for me to illustrate with another bug I'm presently 
working on (has only happend once, I haven't been able to repeat it yet):

Exception in thread "SecurityPolicyWriter policy creation thread" 
java.lang.NullPointerException
     at 
java.security.ProtectionDomain.getPrincipals(ProtectionDomain.java:222)
     at 
org.apache.river.tool.SecurityPolicyWriter$1.run(SecurityPolicyWriter.java:257)
     at java.lang.Thread.run(Thread.java:744)

In this case ProtectionDomain is throwing a NullPointerException, when 
trying to clone a null array.

But it's not possible for the principals array to be null, it's assigned 
in both constructors?

The only way this is possible is through unsafe publication:  After 
construction the ProtectionDomain instance
was shared with another thread, but there was no synchronization, 
volatile, final field fence, publication to a concurrent collection etc.

The principals field in ProtectionDomain is effectively final, once set, 
it's not modified, if the field was declared final, there wouldn't be a bug.

As the developer trying to solve a race condition, I don't have control 
over ProtectionDomain, I can't make this field final, so I must
find where the ProtectionDomain is shared between threads after it has 
been created.  This is difficult to track down.

A downstream developer using mutable unsafely published RemoteEvent, 
could experience a similar bug, suddenly the simple case is no longer 
simple.

SO LETS CONSIDER IT FROM THE ARCHITECT'S VIEW:

An inexperienced developer, creating a RemoteEvent subclass, may simply 
extend it and create their own event type, if it's immutable, they need 
not be concerned about the complexities of state or safe pulication.  
They will not experience any race conditions or other bugs.

If a developer wants a mutable RemoteEvent, they are required to take 
full responsibility for state and must override superclass methods and 
also include the following in their readObject method:

     in.defaultReadObject();
     eventID = super.getID();
     seqNum = super.getSequenceNumber();
     handback = super.getRegistrationObject();

That's it.

RemoteEvent, Jini 1.0, designed around 1998.

The current memory model (which corrected a number of issues) didn't exist.

RemoteEvent is designed for extension.

Can be effectively immutable, but must be safely published before 
sharing with other threads.

Public getter methods provided, no setters.

Has serial form.

Field access was protected.

It had an implicit and undocumented contract, one of publish, but don't 
modify after publication, as it's protected state is available to all 
subclasses and public accessor methods were not synchronized.

The next section is not intended to offend and I do understand it is a 
sensitive topic for some, but it is what I've experienced (relates to 
the 2.2. branch and earlier):

·River contained a lot of latent bugs (Findbugs static analysis 
highlights a lot of these and anyone may run this themselves to 
confirm), however due years of in field deployment and implicit coupling 
between different synchronized blocks, that were not directly related, 
most of these bugs were masked and are not experienced, or in some cases 
it is likely that there are existing workarounds.

·Latent bugs appear when you make changes, the bugs are if not always 
unrelated to the code change; the code was brittle.

While most bugs have been fixed in River 3.0, and visibility between 
threads has been fixed, some cases where changes are not atomic remain, 
for example, see org.apache.river.mahalo.TxnManagerTransaction, see the 
field parts on line 136, since it is an instance of Vector, all access 
is synchronized, however the actions on the state of this object are not 
atomic; other actions may be interleaved, leading to a corruption of 
state, however this doesn't appear to occur in practise.

I could have just made the fields in RemoteEvent volatile protected, but 
that might not be atomic, or what the developer intends.

If RemoteEvent's fields are accessible by all subclasses we cannot 
reason about it's state, so we should make a copy, before transferring 
between threads, however it doesn't support clone and doesn't have 
equals or hashcode contracts, so it can't be copied; we could serialize 
and unserialize it to create a new instance.

A user can for instance, create a queue, with a comparator, to process 
the events he / she receives in order, but a mutating events in an 
ordered queue would break the contract.

So it comes down to safe publication.  Some objects are safely published 
when construction completes, some aren't.

In this case, the simplest use case is an immutable event.

All River's event implementations are effectively immutable.

Rio, does have mutable events, they are built and then published by a 
handler that fires the event.   Rio has a number of event classes that 
extend org.rioproject.event.RemoteServiceEvent, but only 
RemoteServiceEvent itself will require modification, none of it's 
subclasses require modification.

https://github.com/pfirmstone/Rio/blob/master/rio-core/rio-api/src/main/java/org/rioproject/event/RemoteServiceEvent.java

I understand that it may at first appear to be simple to revert the 
fields back to protected, but after significant time spent considering 
all possiblities, it turns out the be the most complex option.

The current implementation in River 3.0 is in fact the simplest most 
flexible option as it allows for both immutable and managaged mutable 
implementations.  When a mutable subclass takes control of state, the 
developer has full control, choices such as synchronized access, partly 
final, or volatile, that would not otherwise be possible.

Regards,

Peter.

On 11/04/2016 1:41 AM, Greg Trasuk wrote:
> Hi Peter:
>
> I must have missed the discussion on the original change to the API (RemoteEvent is intended for extension, so it’s part of the public API).
>
> What’s the reasoning on making the fields immutable?  It would seem simpler to undo the change and make the fields ‘protected’ again, rather than force every subclass to implement the readObject/writeObject mechanism.  RemoteEvent is pretty core to the usage of Jini.  it seems like a pretty onerous requirement, plus I have a feeling that new users would find it awfully confusing.
>
> Cheers,
>
> Greg Trasuk
>
>> On Apr 9, 2016, at 9:57 PM,peter_firmstone@apache.org  wrote:
>>
>> Author: peter_firmstone
>> Date: Sun Apr 10 01:57:46 2016
>> New Revision: 1738402
>>
>> URL:http://svn.apache.org/viewvc?rev=1738402&view=rev
>> Log:
>> Critical patch:
>>
>> This patch provides a compatible upgrade path for existing mutable subclasses of RemoteEvent.
>>
>> Modified:
>>     river/jtsk/trunk/src/net/jini/core/event/RemoteEvent.java
>>
>> Modified: river/jtsk/trunk/src/net/jini/core/event/RemoteEvent.java
>> URL:http://svn.apache.org/viewvc/river/jtsk/trunk/src/net/jini/core/event/RemoteEvent.java?rev=1738402&r1=1738401&r2=1738402&view=diff
>> ==============================================================================
>> --- river/jtsk/trunk/src/net/jini/core/event/RemoteEvent.java (original)
>> +++ river/jtsk/trunk/src/net/jini/core/event/RemoteEvent.java Sun Apr 10 01:57:46 2016
>> @@ -18,6 +18,8 @@
>>
>> package net.jini.core.event;
>>
>> +import java.io.ObjectOutputStream.PutField;
>> +import java.io.ObjectStreamField;
>> import java.rmi.MarshalledObject;
>>
>> /**
>> @@ -81,6 +83,14 @@ import java.rmi.MarshalledObject;
>> public class RemoteEvent extends java.util.EventObject {
>>
>>      private static final long serialVersionUID = 1777278867291906446L;
>> +
>> +    private static final ObjectStreamField[] serialPersistentFields =
>> +	{
>> +	    new ObjectStreamField("source", Object.class),
>> +	    new ObjectStreamField("eventID", long.class),
>> +	    new ObjectStreamField("seqNum", long.class),
>> +	    new ObjectStreamField("handback", MarshalledObject.class)
>> +	};
>>
>>      /**
>>       * The event source.
>> @@ -188,4 +198,39 @@ public class RemoteEvent extends java.ut
>> 	stream.defaultReadObject();
>> 	super.source = source;
>>      }
>> +
>> +    /**
>> +     * In River 3.0.0, RemoteEvent became immutable and all state made private,
>> +     * previously all fields were protected and non final.
>> +     *<p>
>> +     * This change breaks compatibility for subclasses that access these fields
>> +     * directly.  For other classes, all fields were accessible
>> +     * via public API getter methods.
>> +     *<p>
>> +     * To allow an upgrade path for subclasses that extend RemoteEvent and
>> +     * provide public setter methods for these fields, it is recommended to
>> +     * override all public methods and maintain state independently using
>> +     * transient fields.  The subclass should also use RemoteEvent getter
>> +     * methods to set these transient fields during de-serialization.
>> +     *<p>
>> +     * writeObject, instead of writing RemoteEvent fields, writes the
>> +     * result of all getter methods to the ObjectOutputStream, during serialization,
>> +     * preserving serial form compatibility wither earlier versions, while
>> +     * also allowing mutable subclasses to maintain full serial compatibility.
>> +     *<p>
>> +     * Mutable subclasses honoring this contract will be compatible with all
>> +     * versions since Jini 1.0.
>> +     *
>> +     * @param stream
>> +     * @throws java.io.IOException
>> +     */
>> +    private void writeObject(java.io.ObjectOutputStream stream) throws java.io.IOException
>> +    {
>> +	PutField fields = stream.putFields();
>> +	fields.put("source", getSource());
>> +	fields.put("eventID", getID());
>> +	fields.put("seqNum", getSequenceNumber());
>> +	fields.put("handback", getRegistrationObject());
>> +	stream.writeFields();
>> +    }
>> }
>>
>>