You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@felix.apache.org by fm...@apache.org on 2009/08/29 21:44:59 UTC
svn commit: r809194 - in
/felix/trunk/configadmin/src/main/java/org/apache/felix/cm/impl:
ConfigurationImpl.java ConfigurationManager.java
Author: fmeschbe
Date: Sat Aug 29 19:44:58 2009
New Revision: 809194
URL: http://svn.apache.org/viewvc?rev=809194&view=rev
Log:
FELIX-1542 add lastModificationTime and lastUpdateTime fields to track
modifications and updates to prevent the UpdateConfiguration task from
updating properties to ManagedService[Factory] if it has already been
updated upon service registration due to the race condition.
Modified:
felix/trunk/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationImpl.java
felix/trunk/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationManager.java
Modified: felix/trunk/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationImpl.java
URL: http://svn.apache.org/viewvc/felix/trunk/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationImpl.java?rev=809194&r1=809193&r2=809194&view=diff
==============================================================================
--- felix/trunk/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationImpl.java (original)
+++ felix/trunk/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationImpl.java Sat Aug 29 19:44:58 2009
@@ -40,6 +40,51 @@
class ConfigurationImpl extends ConfigurationBase
{
+ /*
+ * Concurrency note: There is a slight (but real) chance of a race condition
+ * between a configuration update and a ManagedService[Factory] registration.
+ * Per the specification a ManagedService must be called with configuration
+ * or null when registered and a ManagedService must be called with currently
+ * existing configuration when registered. Also the ManagedService[Factory]
+ * must be updated when the configuration is updated.
+ *
+ * Consider now this situation of two threads T1 and T2:
+ *
+ * T1. create and update configuration
+ * ConfigurationImpl.update persists configuration and sets field
+ * Thread preempted
+ *
+ * T2. ManagedServiceUpdate constructor reads configuration
+ * Uses configuration already persisted by T1 for update
+ * Schedules task to update service with the configuration
+ *
+ * T1. Runs again creating the UpdateConfiguration task with the
+ * configuration persisted before being preempted
+ * Schedules task to update service
+ *
+ * Update Thread:
+ * Updates ManagedService with configuration prepared by T2
+ * Updates ManagedService with configuration prepared by T1
+ *
+ * The correct behaviour would be here, that the second call to update
+ * would not take place.
+ *
+ * This concurrency safety is implemented with the help of the
+ * lastModificationTime field updated by the configure(Dictionary) method
+ * when setting the properties field and the lastUpdatedTime field updated
+ * in the Update Thread after calling the update(Dictionary) method of
+ * the ManagedService[Factory] service.
+ *
+ * The UpdateConfiguration task compares the lastModificationTime to the
+ * lastUpdateTime. If the configuration has been modified after being
+ * updated the last time, it is updated in the ManagedService[Factory]. If
+ * the configuration has already been updated since being modified (as in
+ * the case above), the UpdateConfiguration thread does not call the update
+ * method (but still sends the CM_UPDATED event).
+ *
+ * See also FELIX-1542.
+ */
+
/**
* The name of a synthetic property stored in the persisted configuration
* data to indicate that the configuration data is new, that is created but
@@ -80,6 +125,30 @@
*/
private volatile boolean isDeleted;
+ /**
+ * Time of last configuration properties update.
+ * This is intended to be managed by the {@link #configure(Dictionary)}
+ * method when setting the properties field.
+ * <p>
+ * This field is set to the current time upon construction and every time
+ * the {@link #configure(Dictionary)} method sets the {@link #properties}
+ * field.
+ */
+ private volatile long lastModificationTime;
+
+ /**
+ * Time of last submission of the configuration properties to the (or more)
+ * ManagedService[Factory] services this is managed by the update thread
+ * and must solely be handled on the Update Thread.
+ * <p>
+ * This field is initialized to -1 in the constructors and is set to the
+ * current time when the configuration properties are supplied to the
+ * ManagedService[Factory] in the Update Thread.
+ *
+ * @see #lastModificationTime
+ */
+ private volatile long lastUpdatedTime;
+
ConfigurationImpl( ConfigurationManager configurationManager, PersistenceManager persistenceManager,
Dictionary properties )
@@ -89,6 +158,7 @@
this.factoryPID = ( String ) properties.remove( ConfigurationAdmin.SERVICE_FACTORYPID );
this.isDeleted = false;
+ this.lastUpdatedTime = -1;
// set the properties internally
configureFromPersistence( properties );
@@ -103,6 +173,8 @@
this.factoryPID = factoryPid;
this.isDeleted = false;
this.properties = null;
+ this.lastModificationTime = System.currentTimeMillis();
+ this.lastUpdatedTime = -1;
// this is a new configuration object, store immediately unless
// the new configuration object is created from a factory, in which
@@ -306,6 +378,42 @@
/**
+ * Returns the time of the last update of this configuration object. In
+ * particular this is the time at which the properties field has been
+ * set to new values.
+ * <p>
+ * This value may be compared to the {@link #getLastUpdatedTime()} to
+ * decide whether to update the ManagedService[Factory] or not.
+ */
+ long getLastModificationTime()
+ {
+ return lastModificationTime;
+ }
+
+ /**
+ * Returns the time of the last update of this configuration to the
+ * subscribing ManagedService or ManagedServiceFactory. This time may be
+ * compared to the {@link #getLastModificationTime()} to decide whether
+ * the configuration should be updated or not.
+ */
+ long getLastUpdatedTime()
+ {
+ return lastUpdatedTime;
+ }
+
+ /**
+ * Sets the time of the last update of this configuration to the
+ * ManagedService or ManagedServiceFactory subscribed to this configuration.
+ * <p>
+ * This method should only be called from the Update Thread after supplying
+ * the configuration to the ManagedService[Factory].
+ */
+ void setLastUpdatedTime( )
+ {
+ this.lastUpdatedTime = System.currentTimeMillis();
+ }
+
+ /**
* Returns <code>false</code> if this configuration contains configuration
* properties. Otherwise <code>true</code> is returned and this is a
* newly creted configuration object whose {@link #update(Dictionary)}
@@ -364,7 +472,11 @@
}
}
- this.properties = newProperties;
+ synchronized ( this )
+ {
+ this.properties = newProperties;
+ this.lastModificationTime = System.currentTimeMillis();
+ }
}
Modified: felix/trunk/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationManager.java
URL: http://svn.apache.org/viewvc/felix/trunk/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationManager.java?rev=809194&r1=809193&r2=809194&view=diff
==============================================================================
--- felix/trunk/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationManager.java (original)
+++ felix/trunk/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationManager.java Sat Aug 29 19:44:58 2009
@@ -1064,7 +1064,7 @@
Bundle serviceBundle = sr.getBundle();
if ( serviceBundle == null )
{
- log( LogService.LOG_INFO, "ServiceFactory for PID " + pid
+ log( LogService.LOG_INFO, "Service for PID " + pid
+ " seems to already have been unregistered, not updating with configuration", null );
return;
}
@@ -1115,6 +1115,12 @@
{
handleCallBackError( t, sr, config );
}
+
+ // update the lastUpdatedTime if there is configuration
+ if ( config != null )
+ {
+ config.setLastUpdatedTime();
+ }
}
public String toString()
@@ -1276,6 +1282,9 @@
{
handleCallBackError( t, sr, cfg );
}
+
+ // update the lastUpdatedTime
+ cfg.setLastUpdatedTime();
}
}
}
@@ -1292,13 +1301,18 @@
private final ConfigurationImpl config;
private final Dictionary properties;
+ private final long lastModificationTime;
private final boolean fireEvent;
UpdateConfiguration( final ConfigurationImpl config, boolean fireEvent )
{
this.config = config;
- this.properties = config.getProperties( true );
+ synchronized ( config )
+ {
+ this.properties = config.getProperties( true );
+ this.lastModificationTime = config.getLastModificationTime();
+ }
this.fireEvent = fireEvent;
}
@@ -1307,6 +1321,18 @@
{
try
{
+ // only update configuration if lastModificationTime is
+ // less than lastUpdateTime
+ // (this must be inside the try-catch-finally to ensure
+ // the event is sent regardless of ManagedService[Factory]
+ // update)
+ if ( lastModificationTime < config.getLastUpdatedTime() )
+ {
+ log( LogService.LOG_DEBUG, "Configuration " + config.getPid()
+ + " has already been updated, nothing to be done anymore.", null );
+ return;
+ }
+
if ( config.getFactoryPid() == null )
{
final ServiceReference[] srList = bundleContext.getServiceReferences( ManagedService.class
@@ -1371,6 +1397,9 @@
{
bundleContext.ungetService( userRef );
}
+
+ // update the lastUpdatedTime
+ config.setLastUpdatedTime();
}
}
}
@@ -1439,6 +1468,9 @@
{
bundleContext.ungetService( ref );
}
+
+ // update the lastUpdatedTime
+ config.setLastUpdatedTime();
}
}
}