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 2007/08/20 15:15:17 UTC
svn commit: r567688 - in
/felix/trunk/configadmin/src/main/java/org/apache/felix/cm:
file/FilePersistenceManager.java impl/ConfigurationAdminImpl.java
impl/ConfigurationImpl.java impl/ConfigurationManager.java
Author: fmeschbe
Date: Mon Aug 20 06:15:16 2007
New Revision: 567688
URL: http://svn.apache.org/viewvc?rev=567688&view=rev
Log:
FELIX-335 Configuration Admin updates with empty properties and throws NullPointerExceptions due to race condition
Modified:
felix/trunk/configadmin/src/main/java/org/apache/felix/cm/file/FilePersistenceManager.java
felix/trunk/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationAdminImpl.java
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/file/FilePersistenceManager.java
URL: http://svn.apache.org/viewvc/felix/trunk/configadmin/src/main/java/org/apache/felix/cm/file/FilePersistenceManager.java?rev=567688&r1=567687&r2=567688&view=diff
==============================================================================
--- felix/trunk/configadmin/src/main/java/org/apache/felix/cm/file/FilePersistenceManager.java (original)
+++ felix/trunk/configadmin/src/main/java/org/apache/felix/cm/file/FilePersistenceManager.java Mon Aug 20 06:15:16 2007
@@ -72,6 +72,29 @@
* <tr><td><code>org.apache.felix.log.LogService</code><td><code>org/apache/felix/log/LogService.config</code></tr>
* <tr><td><code>sample.fläche</code><td><code>sample/fl%00e8che.config</code></tr>
* </table>
+ * <p>
+ * <b>Mulithreading Issues</b>
+ * <p>
+ * In a multithreaded environment the {@link #store(String, Dictionary)} and
+ * {@link #load(File)} methods may be called at the the quasi-same time for the
+ * same configuration PID. It may no happen, that the store method starts
+ * writing the file and the load method might at the same time read from the
+ * file currently being written and thus loading corrupt data (if data is
+ * available at all).
+ * <p>
+ * To prevent this situation from happening, the methods use synchronization
+ * and temporary files as follows:
+ * <ul>
+ * <li>The {@link #store(String, Dictionary)} method writes a temporary file
+ * with file extension <code>.tmp</code>. When done, the file is renamed to
+ * actual configuration file name as implied by the PID. This last step of
+ * renaming the file is synchronized on the FilePersistenceManager instance.</li>
+ * <li>The {@link #load(File)} method is completeley synchronized on the
+ * FilePersistenceManager instance such that the {@link #store} method might
+ * inadvertantly try to replace the file while it is being read.</li>
+ * <li>Finally the <code>Iterator</code> returned by {@link #getDictionaries()}
+ * is implemented such that any temporary configuration file is just ignored.</li>
+ * </ul>
*
* @author fmeschbe
*/
@@ -89,6 +112,14 @@
*/
private static final String FILE_EXT = ".config";
+ /**
+ * The extension of the configuration files, while they are being written
+ * (value is ".tmp").
+ *
+ * @see #store(String, Dictionary)
+ */
+ private static final String TMP_EXT = ".tmp";
+
private static final BitSet VALID_PATH_CHARS;
/**
@@ -389,16 +420,33 @@
public void store( String pid, Dictionary props ) throws IOException
{
OutputStream out = null;
+ File tmpFile = null;
try
{
File cfgFile = getFile( pid );
// ensure parent path
- cfgFile.getParentFile().mkdirs();
+ File cfgDir = cfgFile.getParentFile();
+ cfgDir.mkdirs();
-
- out = new FileOutputStream( cfgFile );
+ // write the configuration to a temporary file
+ tmpFile = File.createTempFile( cfgFile.getName(), TMP_EXT, cfgDir );
+ out = new FileOutputStream( tmpFile );
ConfigurationHandler.write( out, props );
+ out.close();
+
+ // after writing the file, rename it but ensure, that no other
+ // might at the same time open the new file
+ // see load(File)
+ synchronized (this) {
+ // make sure the cfg file does not exists (just for sanity)
+ if (cfgFile.exists()) {
+ cfgFile.delete();
+ }
+
+ // rename the temporary file to the new file
+ tmpFile.renameTo( cfgFile );
+ }
}
finally
{
@@ -413,6 +461,10 @@
// ignore
}
}
+
+ if (tmpFile.exists()) {
+ tmpFile.delete();
+ }
}
}
@@ -430,23 +482,32 @@
*/
private Dictionary load( File cfgFile ) throws IOException
{
- InputStream ins = null;
- try
+ // synchronize this instance to make at least sure, the file is
+ // not at the same time accessed by another thread (see store())
+ // we have to synchronize the complete load time as the store
+ // method might want to replace the file while we are reading and
+ // still have the file open. This might be a problem e.g. in Windows
+ // environments, where files may not be removed which are still open
+ synchronized ( this )
{
- ins = new FileInputStream( cfgFile );
- return ConfigurationHandler.read( ins );
- }
- finally
- {
- if ( ins != null )
+ InputStream ins = null;
+ try
{
- try
- {
- ins.close();
- }
- catch ( IOException ioe )
+ ins = new FileInputStream( cfgFile );
+ return ConfigurationHandler.read( ins );
+ }
+ finally
+ {
+ if ( ins != null )
{
- // ignore
+ try
+ {
+ ins.close();
+ }
+ catch ( IOException ioe )
+ {
+ // ignore
+ }
}
}
}
@@ -536,7 +597,7 @@
{
File cfgFile = fileList[idx++];
- if ( cfgFile.isFile() )
+ if ( cfgFile.isFile() && !cfgFile.getName().endsWith( TMP_EXT ))
{
try
{
Modified: felix/trunk/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationAdminImpl.java
URL: http://svn.apache.org/viewvc/felix/trunk/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationAdminImpl.java?rev=567688&r1=567687&r2=567688&view=diff
==============================================================================
--- felix/trunk/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationAdminImpl.java (original)
+++ felix/trunk/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationAdminImpl.java Mon Aug 20 06:15:16 2007
@@ -93,7 +93,7 @@
*/
public Configuration getConfiguration( String pid ) throws IOException
{
- ConfigurationImpl config = configurationManager.getConfiguration( pid );
+ ConfigurationImpl config = configurationManager.getConfiguration( pid, getBundle().getLocation() );
if ( config.getBundleLocation() == null )
{
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=567688&r1=567687&r2=567688&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 Mon Aug 20 06:15:16 2007
@@ -39,6 +39,24 @@
{
/**
+ * The name of a synthetic property stored in the persisted configuration
+ * data to indicate that the configuration data is new, that is created but
+ * never updated (value is "_felix_.cm.newConfiguration").
+ * <p>
+ * This special property is stored by the
+ * {@link #ConfigurationImpl(ConfigurationManager, PersistenceManager, String, String, String)}
+ * constructor, when the configuration is first created and persisted and is
+ * interpreted by the
+ * {@link #ConfigurationImpl(ConfigurationManager, PersistenceManager, Dictionary)}
+ * method when the configuration data is loaded in a new object.
+ * <p>
+ * The goal of this property is to keep the information on whether
+ * configuration data is new (but persisted as per the spec) or has already
+ * been assigned with possible no data.
+ */
+ private static final String CONFIGURATION_NEW = "_felix_.cm.newConfiguration";
+
+ /**
* The {@link ConfigurationManager configuration manager} instance which
* caused this configuration object to be created.
*/
@@ -83,7 +101,8 @@
* The configuration data of this configuration instance. This is a private
* copy of the properties of which a copy is made when the
* {@link #getProperties()} method is called. This field is <code>null</code> if
- * the configuration has been created and never stored to persistence.
+ * the configuration has been created and never been updated with acutal
+ * configuration properties.
*/
private CaseInsensitiveDictionary properties;
@@ -98,17 +117,26 @@
factoryPID = ( String ) properties.remove( ConfigurationAdmin.SERVICE_FACTORYPID );
bundleLocation = ( String ) properties.remove( ConfigurationAdmin.SERVICE_BUNDLELOCATION );
- configure( properties );
+ // set the properties internally
+ configureFromPersistence( properties );
}
ConfigurationImpl( ConfigurationManager configurationManager, PersistenceManager persistenceManager, String pid,
- String factoryPid )
+ String factoryPid, String bundleLocation ) throws IOException
{
this.configurationManager = configurationManager;
this.persistenceManager = persistenceManager;
this.pid = pid;
- factoryPID = factoryPid;
+ this.factoryPID = factoryPid;
+ this.bundleLocation = bundleLocation;
+ this.properties = null;
+
+ // this is a new configuration object, store immediately
+ Dictionary props = new Hashtable();
+ setAutoProperties( props, true );
+ props.put( CONFIGURATION_NEW, Boolean.TRUE );
+ persistenceManager.store( pid, props );
}
@@ -117,7 +145,7 @@
*/
public void delete() throws IOException
{
- if ( !this.isDeleted() )
+ if ( !isDeleted() )
{
persistenceManager.delete( pid );
persistenceManager = null;
@@ -179,14 +207,14 @@
*/
public void setBundleLocation( String bundleLocation )
{
- if ( !this.isDeleted() )
+ if ( !isDeleted() )
{
this.bundleLocation = bundleLocation;
// 104.15.2.8 The bundle location will be set persistently
try
{
- this.store();
+ store();
}
catch ( IOException ioe )
{
@@ -201,7 +229,7 @@
*/
public void update() throws IOException
{
- if ( !this.isDeleted() )
+ if ( !isDeleted() )
{
// read configuration from persistence (again)
Dictionary properties = persistenceManager.load( pid );
@@ -214,7 +242,7 @@
+ servicePid );
}
- this.configure( properties );
+ configureFromPersistence( properties );
configurationManager.updated( this );
}
@@ -226,15 +254,15 @@
*/
public void update( Dictionary properties ) throws IOException
{
- if ( !this.isDeleted() )
+ if ( !isDeleted() )
{
CaseInsensitiveDictionary newProperties = new CaseInsensitiveDictionary( properties );
- this.setAutoProperties( newProperties, true );
+ setAutoProperties( newProperties, true );
persistenceManager.store( pid, newProperties );
- this.configure( newProperties );
+ configure( newProperties );
configurationManager.updated( this );
}
@@ -295,11 +323,11 @@
props = new Hashtable();
// add automatic properties including the bundle location (if set)
- this.setAutoProperties( props, true );
+ setAutoProperties( props, true );
}
- else if ( this.getBundleLocation() != null )
+ else if ( getBundleLocation() != null )
{
- props.put( ConfigurationAdmin.SERVICE_BUNDLELOCATION, this.getBundleLocation() );
+ props.put( ConfigurationAdmin.SERVICE_BUNDLELOCATION, getBundleLocation() );
}
// only store now, if this is not a new configuration
@@ -307,6 +335,12 @@
}
+ boolean isNew()
+ {
+ return properties == null;
+ }
+
+
boolean isDeleted()
{
if ( persistenceManager != null )
@@ -323,19 +357,33 @@
}
+ private void configureFromPersistence( Dictionary properties )
+ {
+ // if the this is not an empty/new configuration, accept the properties
+ // otherwise just set the properties field to null
+ if ( properties.get( CONFIGURATION_NEW ) == null )
+ {
+ configure( properties );
+ }
+ else
+ {
+ this.properties = null;
+ }
+ }
+
private void configure( Dictionary properties )
{
// remove predefined properties
- this.clearAutoProperties( properties );
+ clearAutoProperties( properties );
// ensure CaseInsensitiveDictionary
if ( properties instanceof CaseInsensitiveDictionary )
{
- this.properties = (CaseInsensitiveDictionary)properties;
+ this.properties = ( CaseInsensitiveDictionary ) properties;
}
else
{
- properties = new CaseInsensitiveDictionary( properties );
+ this.properties = new CaseInsensitiveDictionary( properties );
}
}
@@ -343,13 +391,13 @@
void setAutoProperties( Dictionary properties, boolean withBundleLocation )
{
// set pid and factory pid in the properties
- this.replaceProperty( properties, Constants.SERVICE_PID, pid );
- this.replaceProperty( properties, ConfigurationAdmin.SERVICE_FACTORYPID, factoryPID );
+ replaceProperty( properties, Constants.SERVICE_PID, pid );
+ replaceProperty( properties, ConfigurationAdmin.SERVICE_FACTORYPID, factoryPID );
// bundle location is not set here
if ( withBundleLocation )
{
- this.replaceProperty( properties, ConfigurationAdmin.SERVICE_BUNDLELOCATION, this.getBundleLocation() );
+ replaceProperty( properties, ConfigurationAdmin.SERVICE_BUNDLELOCATION, getBundleLocation() );
}
else
{
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=567688&r1=567687&r2=567688&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 Mon Aug 20 06:15:16 2007
@@ -271,11 +271,18 @@
}
- void cacheConfiguration( ConfigurationImpl configuration )
+ ConfigurationImpl cacheConfiguration( ConfigurationImpl configuration )
{
synchronized ( configurations )
{
+ Object existing = configurations.get( configuration.getPid() );
+ if ( existing != null )
+ {
+ return ( ConfigurationImpl ) existing;
+ }
+
configurations.put( configuration.getPid(), configuration );
+ return configuration;
}
}
@@ -310,8 +317,7 @@
// create the configuration
String pid = createPid( factoryPid );
- ConfigurationImpl config = createConfiguration( pid, factoryPid );
- config.setBundleLocation( configurationAdmin.getBundle().getLocation() );
+ ConfigurationImpl config = createConfiguration( pid, factoryPid, configurationAdmin.getBundle().getLocation() );
// add the configuration to the factory
factory.addPID( pid );
@@ -331,11 +337,7 @@
{
// create the configuration
String pid = createPid( factoryPid );
- ConfigurationImpl config = createConfiguration( pid, factoryPid );
- if ( location != null )
- {
- config.setBundleLocation( location );
- }
+ ConfigurationImpl config = createConfiguration( pid, factoryPid, location );
// add the configuration to the factory
Factory factory = getFactory( factoryPid );
@@ -346,26 +348,41 @@
}
- ConfigurationImpl getConfiguration( String pid ) throws IOException
+ ConfigurationImpl getExistingConfiguration( String pid ) throws IOException
{
- return getConfiguration( pid, true );
+ ConfigurationImpl config = getCachedConfiguration( pid );
+ if ( config != null )
+ {
+ return config;
+ }
+
+ PersistenceManager[] pmList = getPersistenceManagers();
+ for ( int i = 0; i < pmList.length; i++ )
+ {
+ if ( pmList[i].exists( pid ) )
+ {
+ Dictionary props = pmList[i].load( pid );
+ config = new ConfigurationImpl( this, pmList[i], props );
+ return cacheConfiguration( config );
+ }
+ }
+
+ // neither the cache nor any persistence manager has configuration
+ return null;
}
-
-
+
+
ConfigurationImpl getConfiguration( String pid, String bundleLocation ) throws IOException
{
- ConfigurationImpl config = getConfiguration( pid, false );
-
- if ( config == null )
+ // check for existing (cached or persistent) configuration
+ ConfigurationImpl config = getExistingConfiguration( pid );
+ if ( config != null )
{
- config = createConfiguration( pid, null );
- if ( bundleLocation != null )
- {
- config.setBundleLocation( bundleLocation );
- }
+ return config;
}
- return config;
+ // else create new configuration also setting the bundle location
+ return createConfiguration( pid, null, bundleLocation );
}
@@ -577,42 +594,13 @@
}
- ConfigurationImpl getConfiguration( String pid, boolean create ) throws IOException
+ ConfigurationImpl createConfiguration( String pid, String factoryPid, String bundleLocation ) throws IOException
{
- ConfigurationImpl config = getCachedConfiguration( pid );
- if ( config != null )
- {
- return config;
- }
-
- PersistenceManager[] pmList = getPersistenceManagers();
- for ( int i = 0; i < pmList.length; i++ )
- {
- if ( pmList[i].exists( pid ) )
- {
- Dictionary props = pmList[i].load( pid );
- config = new ConfigurationImpl( this, pmList[i], props );
- cacheConfiguration( config );
- return config;
- }
- }
+ // create the configuration (which will also be stored immediately)
+ ConfigurationImpl config = new ConfigurationImpl( this, getPersistenceManagers()[0], pid, factoryPid,
+ bundleLocation );
- // if getting here, there is no configuration yet, optionally create new
- return ( create ) ? createConfiguration( pid, null ) : null;
- }
-
-
- ConfigurationImpl createConfiguration( String pid, String factoryPid ) throws IOException
- {
- ConfigurationImpl config = new ConfigurationImpl( this, getPersistenceManagers()[0], pid, factoryPid );
-
- // immediately store the configuration, yet getProperties() must still
- // return null
- config.store();
-
- cacheConfiguration( config );
-
- return config;
+ return cacheConfiguration( config );
}
@@ -648,9 +636,27 @@
}
+ /**
+ * Calls the registered configuration plugins on the given configuration
+ * object unless the configuration has just been created and not been
+ * updated yet.
+ *
+ * @param sr The service reference of the managed service (factory) which
+ * is to be updated with configuration
+ * @param cfg The configuration object whose properties have to be passed
+ * through the plugins
+ * @return The properties from the configuration object passed through the
+ * plugins or <code>null</code> if the configuration object has
+ * been newly created and no properties exist yet.
+ */
private Dictionary callPlugins( ServiceReference sr, ConfigurationImpl cfg )
{
Dictionary props = cfg.getProperties();
+
+ // guard against NPE for new configuration never updated
+ if (props == null) {
+ return null;
+ }
ServiceReference[] plugins = null;
try
@@ -804,45 +810,66 @@
public void run()
{
+ // get or load configuration for the pid
ConfigurationImpl cfg;
try
{
- cfg = getConfiguration( pid, sr.getBundle().getLocation() );
+ cfg = getExistingConfiguration( pid );
}
catch ( IOException ioe )
{
log( LogService.LOG_ERROR, "Error loading configuration for " + pid, ioe );
return;
}
-
- // 104.3 Ignore duplicate PIDs from other bundles and report them to
- // the log
- // 104.4.1 No update call back for PID already bound to another
- // bundle location
- String bundleLocation = sr.getBundle().getLocation();
- if ( cfg.getBundleLocation() != null && !bundleLocation.equals( cfg.getBundleLocation() ) )
+
+ // this will be set below to be given to the service
+ Dictionary dictionary;
+
+ // check configuration and call plugins if existing and not new
+ if ( cfg != null && !cfg.isNew() )
{
- log( LogService.LOG_ERROR, "Cannot use configuration for " + pid + " requested by bundle "
- + sr.getBundle().getLocation() + " but belongs to " + cfg.getBundleLocation(), null );
- return;
- }
- // 104.3 Report an error in the log if more than one service with
- // the same PID asks for the configuration
- if ( cfg.getServiceReference() != null && !sr.equals( cfg.getServiceReference() ) )
- {
- log( LogService.LOG_ERROR, "Configuration for " + pid + " has already been used for service "
- + cfg.getServiceReference() + " and will now also be given to " + sr, null );
+ // 104.3 Ignore duplicate PIDs from other bundles and report
+ // them to the log
+ // 104.4.1 No update call back for PID already bound to another
+ // bundle location
+ // 104.4.1 assign configuration to bundle if unassigned
+ String bundleLocation = sr.getBundle().getLocation();
+ if ( cfg.getBundleLocation() == null )
+ {
+ cfg.setBundleLocation( bundleLocation );
+ }
+ else if ( !bundleLocation.equals( cfg.getBundleLocation() ) )
+ {
+ log( LogService.LOG_ERROR, "Cannot use configuration for " + pid + " requested by bundle "
+ + sr.getBundle().getLocation() + " but belongs to " + cfg.getBundleLocation(), null );
+ return;
+ }
+
+ // 104.3 Report an error in the log if more than one service
+ // with
+ // the same PID asks for the configuration
+ if ( cfg.getServiceReference() != null && !sr.equals( cfg.getServiceReference() ) )
+ {
+ log( LogService.LOG_ERROR, "Configuration for " + pid + " has already been used for service "
+ + cfg.getServiceReference() + " and will now also be given to " + sr, null );
+ }
+ else
+ {
+ // assign the configuration to the service
+ cfg.setServiceReference( sr );
+ }
+
+ // prepare the configuration for the service (call plugins)
+ dictionary = callPlugins( sr, cfg );
}
else
{
- // assign the configuration to the service
- cfg.setServiceReference( sr );
+ // 104.5.3 ManagedService.updated must be called with null
+ // if no configuration is available
+ dictionary = null;
}
- // prepare the configuration for the service (call plugins)
- Dictionary dictionary = callPlugins( sr, cfg );
-
// update the service with the configuration
try
{
@@ -925,7 +952,7 @@
ConfigurationImpl cfg;
try
{
- cfg = getConfiguration( pid, false );
+ cfg = getExistingConfiguration( pid );
}
catch ( IOException ioe )
{
@@ -942,6 +969,13 @@
factory.storeSilently();
continue;
}
+ else if ( cfg.isNew() )
+ {
+ // Configuration has just been created but not yet updated
+ // we currently just ignore it and have the update mechanism
+ // provide the configuration to the ManagedServiceFactory
+ continue;
+ }
else if ( !factoryPid.equals( cfg.getFactoryPid() ) )
{
log( LogService.LOG_ERROR, "Configuration " + pid + " referred to by factory " + factoryPid
@@ -972,7 +1006,11 @@
// update the service with the configuration
try
{
- service.updated( pid, dictionary );
+ // only, if there is non-null configuration data
+ if ( dictionary != null )
+ {
+ service.updated( pid, dictionary );
+ }
}
catch ( ConfigurationException ce )
{
@@ -1085,7 +1123,11 @@
Dictionary dictionary = callPlugins( sr[0], config );
// update the ManagedServiceFactory with the properties
- srv.updated( config.getPid(), dictionary );
+ // only, if there is non-null configuration data
+ if ( dictionary != null )
+ {
+ srv.updated( config.getPid(), dictionary );
+ }
}
finally
{