You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@river.apache.org by Peter Firmstone <ji...@zeus.net.au> on 2013/05/23 09:40:22 UTC
Don't export a service from within its contructor
No really, don't.
The test failure on arm that has been really driving me nuts for the
last month is finally solved, guess what was the cause of the problem?
That's right unsafe publication.
The "this" reference was published to JERI's ObjectTable prior to
construction completing, other threads could see the object in a
partially constructed state.
This caused problems with synchronization and a lost notifyAll() while
another thread was in a call wait(). notifyAll() was being called every
7ms so it was really being tested.
For more info, see River-420 on Jira.
This goes against a lot of books and literature on how to program Jini,
many examples (including our own) export during construction. This is
no longer how we should create proxy's, we need to get the message out
there. I'm a little short on time for the next few weeks, is anyone
able to publish something on our website?
The good news is I have fixes for all occurrences of exports duing
construction in the main src directory, the bad news is there are a
number of instances in the qa suite still to be fixed, although these
can be fixed relatively easily because they're not public api. The
really bad news is there's going to be a heap of client code out there
that has this issue.
The other good news is the qa refacting build is starting to look very
solid indeed. The last test failure on Arm was actually a segfault in
the JVM in an unrelated test, not our problem, unfortunately I didn't
find a log file or core dump.
Regards,
Peter.
This is what the code looks like now it's fixed, previously the proxy
was created during construction:
/**
* Listener that calls notifyAll on itself
*/
public class LeasedSpaceListener
implements RemoteEventListener, ServerProxyTrust, Serializable
{
private static Logger logger =
Logger.getLogger("com.sun.jini.qa.harness");
private boolean received = false;
private Object proxy;
private final Exporter exporter;
private final AccessControlContext context;
public LeasedSpaceListener(Configuration c) throws RemoteException {
try {
Exporter exporter = QAConfig.getDefaultExporter();
if (c instanceof com.sun.jini.qa.harness.QAConfiguration) {
exporter =
(Exporter) c.getEntry("test",
"outriggerListenerExporter",
Exporter.class);
}
this.exporter = exporter;
context = AccessController.getContext();
// Proxy was originally exported here, allowing "this" to escape.
} catch (ConfigurationException e) {
throw new RemoteException("Bad configuration", e);
}
}
private synchronized Object getProxy(){
if (proxy == null) {
proxy = AccessController.doPrivileged(new
PrivilegedAction<Object>(){
@Override
public Object run() {
try {
return exporter.export(LeasedSpaceListener.this);
} catch (ExportException ex) {
String message = "Proxy export failed for
LeaseListener";
logger.log(Level.WARNING, message , ex);
return null;
}
}
}, context);
}
return proxy;
}
public Object writeReplace() throws ObjectStreamException {
return getProxy();
}
public TrustVerifier getProxyVerifier() {
return new BasicProxyTrustVerifier(getProxy());
}
public void notify(RemoteEvent theEvent)
throws UnknownEventException, RemoteException {
// Perform logging outside the synchronized block so we don't
affect
// timing.
java.util.Date date = new java.util.Date();
synchronized (this){
received = true;
this.notifyAll();
}
logger.log(Level.INFO, "notify called at {0}", date.getTime());
}
/**
* @return the received
*/
public synchronized boolean isReceived() {
return received;
}
/**
* @param received the received to set
*/
public synchronized void setReceived(boolean received) {
this.received = received;
}
}
Re: Don't export a service from within its contructor
Posted by Peter <ji...@zeus.net.au>.
Sounds like a good proposal, River 3.0.
Thoughts?
Peter.
----- Original message -----
> My "startnow" project on java.net, has always provide a "separation" of
> construction and start of the service export. However, the usage of that API is
> supposed to be something like the following.
>
> public class MyService extends PersistentJiniService {
> public static void main( String args[] ) {
> MyService svc = new MyService(args);
> svc.start();
> }
> private MyService( String args[] ) {
> super( args ); // creates the Configuration instance etc.
> }
> }
>
> However, because of the use of the ServiceStarter framework, I typically instead
> have service startup happen as show below,
>
> public class MyService extends PersistentJiniService {
> public MyService( String args[], Lifecycle life ) {
> super(args);
> start();
> }
> }
>
> because ServiceStarter has no other means for starting services. With the work
> that you've done to provide that API in ServiceStarter, I'd say that we need to
> make an API change and a major version rev to demand that there actually be an
> interface implemented by classes provided to ServiceStarter, in all cases.
>
> For some number of years, I've tried to encourage everyone providing a platform
> for service startup/registration, to agree on a common base API that would
> provide portability between containers at some level, which would allow everyone
> to try the various containers, and then perhaps we'd be able to really explore
> the features that each provides.
>
> Gregg Wonderly
>
> On May 23, 2013, at 2:40 AM, Peter Firmstone <ji...@zeus.net.au> wrote:
>
> > No really, don't.
> >
> > The test failure on arm that has been really driving me nuts for the last
> > month is finally solved, guess what was the cause of the problem?
> >
> > That's right unsafe publication.
> >
> > The "this" reference was published to JERI's ObjectTable prior to construction
> > completing, other threads could see the object in a partially constructed
> > state.
> >
> > This caused problems with synchronization and a lost notifyAll() while another
> > thread was in a call wait(). notifyAll() was being called every 7ms so it was
> > really being tested.
> >
> > For more info, see River-420 on Jira.
> >
> > This goes against a lot of books and literature on how to program Jini, many
> > examples (including our own) export during construction. This is no longer
> > how we should create proxy's, we need to get the message out there. I'm a
> > little short on time for the next few weeks, is anyone able to publish
> > something on our website?
> >
> > The good news is I have fixes for all occurrences of exports duing
> > construction in the main src directory, the bad news is there are a number of
> > instances in the qa suite still to be fixed, although these can be fixed
> > relatively easily because they're not public api. The really bad news is
> > there's going to be a heap of client code out there that has this issue.
> >
> > The other good news is the qa refacting build is starting to look very solid
> > indeed. The last test failure on Arm was actually a segfault in the JVM in an
> > unrelated test, not our problem, unfortunately I didn't find a log file or
> > core dump.
> >
> > Regards,
> >
> > Peter.
> >
> >
> > This is what the code looks like now it's fixed, previously the proxy was
> > created during construction:
> >
> > /**
> > * Listener that calls notifyAll on itself
> > */
> > public class LeasedSpaceListener
> > implements RemoteEventListener, ServerProxyTrust, Serializable
> > {
> > private static Logger logger = Logger.getLogger("com.sun.jini.qa.harness");
> > private boolean received = false;
> > private Object proxy;
> > private final Exporter exporter;
> > private final AccessControlContext context;
> >
> > public LeasedSpaceListener(Configuration c) throws RemoteException {
> > try {
> > Exporter exporter = QAConfig.getDefaultExporter();
> > if (c instanceof com.sun.jini.qa.harness.QAConfiguration) {
> > exporter =
> > (Exporter) c.getEntry("test",
> > "outriggerListenerExporter",
> > Exporter.class);
> > }
> > this.exporter = exporter;
> > context = AccessController.getContext();
> > // Proxy was originally exported here, allowing "this" to escape.
> > } catch (ConfigurationException e) {
> > throw new RemoteException("Bad configuration", e);
> > }
> > }
> >
> > private synchronized Object getProxy(){
> > if (proxy == null) {
> > proxy = AccessController.doPrivileged(new
> > PrivilegedAction<Object>(){
> >
> > @Override
> > public Object run() {
> > try {
> > return exporter.export(LeasedSpaceListener.this);
> > } catch (ExportException ex) {
> > String message = "Proxy export failed for
> > LeaseListener"; logger.log(Level.WARNING, message , ex);
> > return null;
> > }
> > }
> >
> > }, context);
> > }
> > return proxy;
> > }
> >
> > public Object writeReplace() throws ObjectStreamException {
> > return getProxy();
> > }
> >
> > public TrustVerifier getProxyVerifier() {
> > return new BasicProxyTrustVerifier(getProxy());
> > }
> >
> > public void notify(RemoteEvent theEvent)
> > throws UnknownEventException, RemoteException {
> > // Perform logging outside the synchronized block so we don't affect
> > // timing.
> > java.util.Date date = new java.util.Date();
> > synchronized (this){
> > received = true;
> > this.notifyAll();
> > }
> > logger.log(Level.INFO, "notify called at {0}", date.getTime());
> > }
> >
> > /**
> > * @return the received
> > */
> > public synchronized boolean isReceived() {
> > return received;
> > }
> >
> > /**
> > * @param received the received to set
> > */
> > public synchronized void setReceived(boolean received) {
> > this.received = received;
> > }
> > }
> >
>
Re: Don't export a service from within its contructor
Posted by Gregg Wonderly <gr...@wonderly.org>.
My "startnow" project on java.net, has always provide a "separation" of construction and start of the service export. However, the usage of that API is supposed to be something like the following.
public class MyService extends PersistentJiniService {
public static void main( String args[] ) {
MyService svc = new MyService(args);
svc.start();
}
private MyService( String args[] ) {
super( args ); // creates the Configuration instance etc.
}
}
However, because of the use of the ServiceStarter framework, I typically instead have service startup happen as show below,
public class MyService extends PersistentJiniService {
public MyService( String args[], Lifecycle life ) {
super(args);
start();
}
}
because ServiceStarter has no other means for starting services. With the work that you've done to provide that API in ServiceStarter, I'd say that we need to make an API change and a major version rev to demand that there actually be an interface implemented by classes provided to ServiceStarter, in all cases.
For some number of years, I've tried to encourage everyone providing a platform for service startup/registration, to agree on a common base API that would provide portability between containers at some level, which would allow everyone to try the various containers, and then perhaps we'd be able to really explore the features that each provides.
Gregg Wonderly
On May 23, 2013, at 2:40 AM, Peter Firmstone <ji...@zeus.net.au> wrote:
> No really, don't.
>
> The test failure on arm that has been really driving me nuts for the last month is finally solved, guess what was the cause of the problem?
>
> That's right unsafe publication.
>
> The "this" reference was published to JERI's ObjectTable prior to construction completing, other threads could see the object in a partially constructed state.
>
> This caused problems with synchronization and a lost notifyAll() while another thread was in a call wait(). notifyAll() was being called every 7ms so it was really being tested.
>
> For more info, see River-420 on Jira.
>
> This goes against a lot of books and literature on how to program Jini, many examples (including our own) export during construction. This is no longer how we should create proxy's, we need to get the message out there. I'm a little short on time for the next few weeks, is anyone able to publish something on our website?
>
> The good news is I have fixes for all occurrences of exports duing construction in the main src directory, the bad news is there are a number of instances in the qa suite still to be fixed, although these can be fixed relatively easily because they're not public api. The really bad news is there's going to be a heap of client code out there that has this issue.
>
> The other good news is the qa refacting build is starting to look very solid indeed. The last test failure on Arm was actually a segfault in the JVM in an unrelated test, not our problem, unfortunately I didn't find a log file or core dump.
>
> Regards,
>
> Peter.
>
>
> This is what the code looks like now it's fixed, previously the proxy was created during construction:
>
> /**
> * Listener that calls notifyAll on itself
> */
> public class LeasedSpaceListener
> implements RemoteEventListener, ServerProxyTrust, Serializable
> {
> private static Logger logger = Logger.getLogger("com.sun.jini.qa.harness");
> private boolean received = false;
> private Object proxy;
> private final Exporter exporter;
> private final AccessControlContext context;
>
> public LeasedSpaceListener(Configuration c) throws RemoteException {
> try {
> Exporter exporter = QAConfig.getDefaultExporter();
> if (c instanceof com.sun.jini.qa.harness.QAConfiguration) {
> exporter =
> (Exporter) c.getEntry("test",
> "outriggerListenerExporter",
> Exporter.class);
> }
> this.exporter = exporter;
> context = AccessController.getContext();
> // Proxy was originally exported here, allowing "this" to escape.
> } catch (ConfigurationException e) {
> throw new RemoteException("Bad configuration", e);
> }
> }
>
> private synchronized Object getProxy(){
> if (proxy == null) {
> proxy = AccessController.doPrivileged(new PrivilegedAction<Object>(){
>
> @Override
> public Object run() {
> try {
> return exporter.export(LeasedSpaceListener.this);
> } catch (ExportException ex) {
> String message = "Proxy export failed for LeaseListener";
> logger.log(Level.WARNING, message , ex);
> return null;
> }
> }
>
> }, context);
> }
> return proxy;
> }
>
> public Object writeReplace() throws ObjectStreamException {
> return getProxy();
> }
>
> public TrustVerifier getProxyVerifier() {
> return new BasicProxyTrustVerifier(getProxy());
> }
>
> public void notify(RemoteEvent theEvent)
> throws UnknownEventException, RemoteException {
> // Perform logging outside the synchronized block so we don't affect
> // timing.
> java.util.Date date = new java.util.Date();
> synchronized (this){
> received = true;
> this.notifyAll();
> }
> logger.log(Level.INFO, "notify called at {0}", date.getTime());
> }
>
> /**
> * @return the received
> */
> public synchronized boolean isReceived() {
> return received;
> }
>
> /**
> * @param received the received to set
> */
> public synchronized void setReceived(boolean received) {
> this.received = received;
> }
> }
>