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_firmst...@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();
+ }
}
