The changes look fine to me. Review: Approved. I'm 110 minutes late.
Sorry...
the names seem a bit unwieldy -- (e.g.
LoadBalancerApplicationRefRegistrationListener)
Lloyd L Chambers wrote:
> Please review by: 7pm March 12 2007
>
> https://glassfish.dev.java.net/issues/show_bug.cgi?id=1961
>
> This change is required to make the base class thread safe. The
> problem was "unsafe publication" (see Java Concurrency in Practice).
> The problem was that NotificationListenerBase would register itself as
> a listener prior to exiting its constructor.
>
> The change touches 11 files, but the logic is not affected; only the
> constructor signature and newInstance() methods (see below).
>
> I was unable to find a solution to making the class thread safe which
> could be safely made without risking incorrect behavior; clients
> instantiating a NotificationListenerBase or any of its subclasses must
> call startListening() *explicitly* now.
>
> To prevent code from compiling which would appear to be OK, but not
> function corrrectly at runtime:
>
> - The signature of the constructor for NotificationListenerBase has
> changed to include a "name" parameter. Named listeners are useful in
> general (no names before), but the inclusion of 'name' also serves the
> purpose of requring a different signature for the constructor, to
> ensure that dependent code is found and fixed. All subclasses have
> been modified accordingly.
>
> - To forestall incorrect usage as much as possible, constructors of
> NotificationListenerBase, MBeanRegistrationListener and all subclasses
> are now 'private', with corresponding public static newInstance()
> methods, which make the necessary call to startListening().
>
> Lloyd
>
>
> cvs server: Diffing
> appserv-api/src/java/com/sun/appserv/management/util/jmx
> Index:
> appserv-api/src/java/com/sun/appserv/management/util/jmx/MBeanRegistrationListener.java
>
> ===================================================================
> RCS file:
> /cvs/glassfish/appserv-api/src/java/com/sun/appserv/management/util/jmx/MBeanRegistrationListener.java,v
>
> retrieving revision 1.1
> diff -u -w -r1.1 MBeanRegistrationListener.java
> ---
> appserv-api/src/java/com/sun/appserv/management/util/jmx/MBeanRegistrationListener.java
> 2 Dec 2006 06:04:06 -0000 1.1
> +++
> appserv-api/src/java/com/sun/appserv/management/util/jmx/MBeanRegistrationListener.java
> 12 Mar 2007 20:38:55 -0000
> @@ -59,14 +59,14 @@
> @param conn
> @param constrain optional fixed or pattern ObjectName
> */
> - public
> + protected
> MBeanRegistrationListener(
> + final String name,
> final MBeanServerConnection conn,
> final ObjectName constrain )
> throws InstanceNotFoundException, IOException
> {
> - super( conn,
> - JMXUtil.getMBeanServerDelegateObjectName() );
> + super( name, conn,
> JMXUtil.getMBeanServerDelegateObjectName() );
> mRegUnregFilter = constrain;
>
> mDefaultDomain = conn.getDefaultDomain();
> @@ -76,11 +76,13 @@
> Calls this( conn, null ).
> @param conn
> */
> - public
> - MBeanRegistrationListener( final MBeanServerConnection conn)
> + protected
> + MBeanRegistrationListener(
> + final String name,
> + final MBeanServerConnection conn)
> throws InstanceNotFoundException, IOException
> {
> - this( conn, (ObjectName)null );
> + this( name, conn, (ObjectName)null );
> }
>
> protected abstract void mbeanRegistered( final ObjectName
> objectName );
> Index:
> appserv-api/src/java/com/sun/appserv/management/util/jmx/NotificationListenerBase.java
>
> ===================================================================
> RCS file:
> /cvs/glassfish/appserv-api/src/java/com/sun/appserv/management/util/jmx/NotificationListenerBase.java,v
>
> retrieving revision 1.1
> diff -u -w -r1.1 NotificationListenerBase.java
> ---
> appserv-api/src/java/com/sun/appserv/management/util/jmx/NotificationListenerBase.java
> 2 Dec 2006 06:04:14 -0000 1.1
> +++
> appserv-api/src/java/com/sun/appserv/management/util/jmx/NotificationListenerBase.java
> 12 Mar 2007 20:38:55 -0000
> @@ -54,6 +54,7 @@
> public abstract class NotificationListenerBase
> implements NotificationListener
> {
> + private final String mName;
> private final MBeanServerConnection mConn;
>
> /** actual MBean ObjectNames, not patterns */
> @@ -66,37 +67,47 @@
> private RegistrationListener mDelegateListener;
> + private volatile boolean mSetupListening;
> +
> /**
> Calls this( conn, listenTo, null, null ).
> + <p><b>Instantiating code must call setupListening() in order
> to initiate
> + listening</b>
> */
> - public
> + protected
> NotificationListenerBase(
> + final String name,
> final MBeanServerConnection conn,
> final ObjectName pattern )
> throws InstanceNotFoundException, IOException
> {
> - this( conn, pattern, null );
> + this( name, conn, pattern, null );
> }
> /**
> - Listen to all MBean(s) which match the pattern
> - 'listenTo'
> + Listen to all MBean(s) which match the pattern 'listenTo'.
> + <p><b>Instantiating code must call setupListening() in order
> to initiate
> + listening</b>
> + @param listenerName arbitrary name of this listener
> @param conn the MBeanServerConnection or MBeanServer
> @param pattern an MBean ObjectName, or an ObjectName pattern
> @param filter optional NotificationFilter
> */
> - public
> + protected
> NotificationListenerBase(
> + final String name,
> final MBeanServerConnection conn,
> final ObjectName pattern,
> final NotificationFilter filter )
> throws InstanceNotFoundException, IOException
> {
> + mName = name;
> mConn = conn;
> mPattern = pattern;
> mFilter = filter;
> mHandback = null;
> mDelegateListener = null;
> + mSetupListening = false;
>
> mListenees = Collections.synchronizedSet(
> new HashSet<ObjectName>() );
> @@ -105,8 +116,6 @@
> {
> throw new IllegalArgumentException();
> }
> -
> - setupListening( );
> }
> /**
> @@ -128,10 +137,15 @@
> }
> }
> - private void
> - setupListening()
> + public synchronized void
> + startListening()
> throws InstanceNotFoundException, IOException
> {
> + if ( mSetupListening )
> + {
> + throw new IllegalStateException( "setupListening() must
> be called exactly once" );
> + }
> +
> if ( mPattern.isPattern() )
> {
> // it's crucial we listen for
> registration/unregistration events
> @@ -162,6 +176,8 @@
> listenToMBean( objectName );
> }
> }
> +
> + mSetupListening = true;
> }
> /**
>
> Index:
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/DomainRootImpl.java
>
> ===================================================================
> RCS file:
> /cvs/glassfish/admin/mbeanapi-impl/src/java/com/sun/enterprise/management/DomainRootImpl.java,v
>
> retrieving revision 1.20
> diff -u -w -r1.20 DomainRootImpl.java
> ---
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/DomainRootImpl.java
> 25 Dec 2005 03:38:51 -0000 1.20
> +++
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/DomainRootImpl.java
> 12 Mar 2007 20:38:55 -0000
> @@ -42,7 +42,7 @@
>
> try
> {
> - new LBBootstrapUtil(getObjectName(), mServer);
> + LBBootstrapUtil.newInstance( "DomainRootImpl",
> getObjectName(), mServer);
> }
> catch (Exception ex)
> {
> cvs server: Diffing
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/config
> Index:
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/config/ConfigFactory.java
>
> ===================================================================
> RCS file:
> /cvs/glassfish/admin/mbeanapi-impl/src/java/com/sun/enterprise/management/config/ConfigFactory.java,v
>
> retrieving revision 1.16
> diff -u -w -r1.16 ConfigFactory.java
> ---
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/config/ConfigFactory.java
> 10 Jan 2007 04:22:37 -0000 1.16
> +++
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/config/ConfigFactory.java
> 12 Mar 2007 20:38:55 -0000
> @@ -198,7 +198,11 @@
> WaitForUnregistrationListener listener = null;
> try
> {
> - listener = new WaitForUnregistrationListener(
> objectName );
> + final String name =
> + "WaitForUnregistrationListener for " +
> JMXUtil.toString(objectName);
> +
> + listener = new WaitForUnregistrationListener( name,
> objectName );
> + listener.startListening();
> return listener;
> }
> catch( Exception e )
> @@ -215,10 +219,12 @@
> private final CountDownLatch mLatch;
> private final boolean mWasUnregistered = false;
> - private WaitForUnregistrationListener( final ObjectName
> objectName )
> + private WaitForUnregistrationListener(
> + final String name,
> + final ObjectName objectName )
> throws InstanceNotFoundException, java.io.IOException
> {
> - super( getMBeanServer(), null );
> + super( name, getMBeanServer(), null );
> mTarget = objectName;
> mLatch = new CountDownLatch(1);
> mStart = System.currentTimeMillis();
>
>
>
> Index:
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LBBaseMBeanRegistrationListener.java
>
> ===================================================================
> RCS file:
> /cvs/glassfish/admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LBBaseMBeanRegistrationListener.java,v
>
> retrieving revision 1.6
> diff -u -w -r1.6 LBBaseMBeanRegistrationListener.java
> ---
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LBBaseMBeanRegistrationListener.java
> 17 Mar 2006 03:34:20 -0000 1.6
> +++
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LBBaseMBeanRegistrationListener.java
> 12 Mar 2007 20:38:55 -0000
> @@ -90,22 +90,17 @@
> * Convenience base class for performing registrations
> * for various load balancer related monitors.
> */
> -public abstract class LBBaseMBeanRegistrationListener
> +abstract class LBBaseMBeanRegistrationListener
> extends MBeanRegistrationListener {
>
> final ObjectNames objectNames = ObjectNames.getInstance(JMX_DOMAIN);
>
> - public LBBaseMBeanRegistrationListener(
> - final MBeanServer mBeanServer, final ObjectName constraint )
> + protected LBBaseMBeanRegistrationListener(
> + final String name,
> + final MBeanServer mBeanServer)
> throws InstanceNotFoundException, IOException {
>
> - super(mBeanServer, constraint);
> - }
> -
> - public LBBaseMBeanRegistrationListener(final MBeanServer
> mBeanServer)
> - throws InstanceNotFoundException, IOException {
> -
> - this(mBeanServer, (ObjectName)null );
> + super( name, mBeanServer);
> }
>
> public void mbeanUnregistered(final ObjectName objectName) {}
> Index:
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LBBootstrapUtil.java
>
> ===================================================================
> RCS file:
> /cvs/glassfish/admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LBBootstrapUtil.java,v
>
> retrieving revision 1.3
> diff -u -w -r1.3 LBBootstrapUtil.java
> ---
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LBBootstrapUtil.java
> 25 Dec 2005 03:40:42 -0000 1.3
> +++
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LBBootstrapUtil.java
> 12 Mar 2007 20:38:55 -0000
> @@ -81,11 +81,28 @@
> LoadBalancerRegistrationListener lbrl = null;
> final private ObjectNames objectNames =
> ObjectNames.getInstance(JMX_DOMAIN);
>
> - public LBBootstrapUtil(ObjectName domainRoot, MBeanServer
> mBeanServer)
> + private LBBootstrapUtil(
> + final String name,
> + final ObjectName domainRoot,
> + final MBeanServer mBeanServer)
> throws InstanceNotFoundException, IOException {
> - super(mBeanServer, domainRoot, null);
> + super( name, mBeanServer, domainRoot, null );
> }
>
> + public static LBBootstrapUtil
> + newInstance(
> + final String name,
> + final ObjectName domainRoot,
> + final MBeanServer server ) throws
> InstanceNotFoundException, IOException
> + {
> + final LBBootstrapUtil listener =
> + new LBBootstrapUtil( name, domainRoot, server );
> +
> + listener.startListening();
> + return listener;
> + }
> +
> +
> public void handleNotification(
> final Notification notifIn, final Object handback) {
> try {
> @@ -103,7 +120,8 @@
> //element registrations. I store the handle of the root LB
> //listener primarily to reuse its ability to enable /
> disable monitoring
> - lbrl = new
> LoadBalancerRegistrationListener((MBeanServer)getConn());
> + lbrl = LoadBalancerRegistrationListener.newInstance(
> + "LBBootstrapUtil root LB listener",
> (MBeanServer)getConn());
> initRestOfLBMBeanRegistrationListeners();
> DomainConfig domainConfig =
> ProxyFactory.getInstance(getConn())
> @@ -146,9 +164,12 @@
>
> MBeanServer mbeanServer = (MBeanServer)getConn();
> try {
> - new LoadBalancerClusterRefRegistrationListener(mbeanServer);
> - new LoadBalancerServerRefRegistrationListener(mbeanServer);
> - new
> LoadBalancerApplicationRefRegistrationListener(mbeanServer);
> + LoadBalancerClusterRefRegistrationListener.newInstance(
> + "LoadBalancerClusterRefRegistrationListener global
> listener", mbeanServer);
> + LoadBalancerServerRefRegistrationListener.newInstance(
> + "LoadBalancerServerRefRegistrationListener global
> listener", mbeanServer);
> + LoadBalancerApplicationRefRegistrationListener.newInstance(
> + "LoadBalancerApplicationRefRegistrationListener
> global listener", mbeanServer);
> } catch(Exception e) {
> throw new RuntimeException(e);
> }
> Index:
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LoadBalancerApplicationRefRegistrationListener.java
>
> ===================================================================
> RCS file:
> /cvs/glassfish/admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LoadBalancerApplicationRefRegistrationListener.java,v
>
> retrieving revision 1.5
> diff -u -w -r1.5 LoadBalancerApplicationRefRegistrationListener.java
> ---
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LoadBalancerApplicationRefRegistrationListener.java
> 17 Mar 2006 03:34:20 -0000 1.5
> +++
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LoadBalancerApplicationRefRegistrationListener.java
> 12 Mar 2007 20:38:55 -0000
> @@ -89,9 +89,23 @@
> public class LoadBalancerApplicationRefRegistrationListener
> extends LBBaseMBeanRegistrationListener {
> - public LoadBalancerApplicationRefRegistrationListener(MBeanServer
> mBeanServer)
> + private LoadBalancerApplicationRefRegistrationListener(
> + final String name,
> + final MBeanServer mBeanServer)
> throws InstanceNotFoundException, IOException {
> - super(mBeanServer);
> + super( name, mBeanServer);
> + }
> +
> + public static LoadBalancerApplicationRefRegistrationListener
> + newInstance(
> + final String name,
> + final MBeanServer conn ) throws InstanceNotFoundException,
> IOException
> + {
> + final LoadBalancerApplicationRefRegistrationListener listener =
> + new LoadBalancerApplicationRefRegistrationListener( name,
> conn );
> +
> + listener.startListening();
> + return listener;
> }
> protected void mbeanRegistered(final ObjectName objectName) {
> Index:
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LoadBalancerClusterRefRegistrationListener.java
>
> ===================================================================
> RCS file:
> /cvs/glassfish/admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LoadBalancerClusterRefRegistrationListener.java,v
>
> retrieving revision 1.5
> diff -u -w -r1.5 LoadBalancerClusterRefRegistrationListener.java
> ---
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LoadBalancerClusterRefRegistrationListener.java
> 9 Mar 2006 20:30:47 -0000 1.5
> +++
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LoadBalancerClusterRefRegistrationListener.java
> 12 Mar 2007 20:38:55 -0000
> @@ -93,9 +93,22 @@
> public class LoadBalancerClusterRefRegistrationListener
> extends LBBaseMBeanRegistrationListener {
>
> - public LoadBalancerClusterRefRegistrationListener(MBeanServer
> mBeanServer)
> + private LoadBalancerClusterRefRegistrationListener(
> + final String name,
> + final MBeanServer mBeanServer)
> throws InstanceNotFoundException, IOException {
> - super(mBeanServer);
> + super( name, mBeanServer);
> + }
> +
> + public static LoadBalancerClusterRefRegistrationListener
> + newInstance(
> + final String name,
> + final MBeanServer conn ) throws InstanceNotFoundException,
> IOException {
> + final LoadBalancerClusterRefRegistrationListener listener =
> + new LoadBalancerClusterRefRegistrationListener( name,
> conn );
> +
> + listener.startListening();
> + return listener;
> }
> protected void mbeanRegistered(final ObjectName objectName) {
> Index:
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LoadBalancerRegistrationListener.java
>
> ===================================================================
> RCS file:
> /cvs/glassfish/admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LoadBalancerRegistrationListener.java,v
>
> retrieving revision 1.9
> diff -u -w -r1.9 LoadBalancerRegistrationListener.java
> ---
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LoadBalancerRegistrationListener.java
> 17 Mar 2006 03:34:20 -0000 1.9
> +++
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LoadBalancerRegistrationListener.java
> 12 Mar 2007 20:38:55 -0000
> @@ -84,13 +84,25 @@
> * @see LoadBalancerApplicationRefRegistrationListener
> * @see LoadBalancerRegistrationListener
> */
> -public class LoadBalancerRegistrationListener extends
> LBBaseMBeanRegistrationListener {
> +public final class LoadBalancerRegistrationListener extends
> LBBaseMBeanRegistrationListener {
>
> final static String MONITORING_ENABLED = "MonitoringEnabled";
> - public LoadBalancerRegistrationListener(MBeanServer mBeanServer)
> + private LoadBalancerRegistrationListener(
> + final String name,
> + final MBeanServer mBeanServer)
> throws InstanceNotFoundException, IOException {
> - super(mBeanServer);
> + super( name, mBeanServer);
> + }
> +
> + public static LoadBalancerRegistrationListener
> + newInstance( final String name, final MBeanServer conn )
> + throws InstanceNotFoundException, IOException {
> + final LoadBalancerRegistrationListener listener =
> + new LoadBalancerRegistrationListener( name, conn );
> +
> + listener.startListening();
> + return listener;
> }
> LoadBalancer registerLoadBalancer(String name) {
> Index:
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LoadBalancerServerRefRegistrationListener.java
>
> ===================================================================
> RCS file:
> /cvs/glassfish/admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LoadBalancerServerRefRegistrationListener.java,v
>
> retrieving revision 1.6
> diff -u -w -r1.6 LoadBalancerServerRefRegistrationListener.java
> ---
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LoadBalancerServerRefRegistrationListener.java
> 17 Mar 2006 03:34:20 -0000 1.6
> +++
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LoadBalancerServerRefRegistrationListener.java
> 12 Mar 2007 20:38:55 -0000
> @@ -81,13 +81,28 @@
> * @see LoadBalancerApplicationRefRegistrationListener
> * @see LoadBalancerRegistrationListener
> */
> -public class LoadBalancerServerRefRegistrationListener
> +public final class LoadBalancerServerRefRegistrationListener
> extends LBBaseMBeanRegistrationListener {
>
> - public LoadBalancerServerRefRegistrationListener(MBeanServer
> mBeanServer)
> + private LoadBalancerServerRefRegistrationListener(
> + final String name,
> + final MBeanServer mBeanServer)
> throws InstanceNotFoundException, IOException {
> - super(mBeanServer);
> + super( name, mBeanServer);
> }
> +
> + public static LoadBalancerServerRefRegistrationListener
> + newInstance(
> + final String name,
> + final MBeanServer conn )
> + throws InstanceNotFoundException, IOException {
> + final LoadBalancerServerRefRegistrationListener listener =
> + new LoadBalancerServerRefRegistrationListener( name, conn );
> +
> + listener.startListening();
> + return listener;
> + }
> +
> protected void mbeanRegistered(final ObjectName objectName) {
> try {
> Index:
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LoaderOfOldConfig.java
>
> ===================================================================
> RCS file:
> /cvs/glassfish/admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LoaderOfOldConfig.java,v
>
> retrieving revision 1.9
> diff -u -w -r1.9 LoaderOfOldConfig.java
> ---
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LoaderOfOldConfig.java
> 9 Jan 2007 23:00:35 -0000 1.9
> +++
> admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/LoaderOfOldConfig.java
> 12 Mar 2007 20:38:55 -0000
> @@ -94,7 +94,8 @@
> try
> {
> - mServerRefListener = new ServerRefListener();
> + mServerRefListener = new ServerRefListener(
> "LoaderOfOldConfig" );
> + mServerRefListener.startListening();
> }
> catch( Exception e )
> {
> @@ -118,10 +119,10 @@
> private final class ServerRefListener
> extends MBeanRegistrationListener
> {
> - public ServerRefListener( )
> + public ServerRefListener( final String name )
> throws InstanceNotFoundException, IOException
> {
> - super( getMBeanServer(),
> getListenToServerRefConfigPattern() );
> + super( name, getMBeanServer(),
> getListenToServerRefConfigPattern() );
> }
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: admin-unsubscribe_at_glassfish.dev.java.net
> For additional commands, e-mail: admin-help_at_glassfish.dev.java.net
>