In general this looks like a big improvement to me but I don't completely understand your strategy. I'm not sure we need the additional lock object. In my local changes I was using "configurations" as the lock for all access to configurations, reloadingConfiguration, and configurationModel.
I still have to look into the configurationModel to see if additional locking is needed. I'm also not sure that a single reloadingConfiguration is still appropriate since there could now be several threads reloading configurations concurrently. I'm not entirely sure we should keep the reloading code.... if we won't be able to do something equivalent using plain osgi then there's little point preserving the functionality now. Anyway, it's good to know this fixes at least the last deadlock we saw. thanks!! david jencks On Jan 29, 2011, at 8:51 PM, [email protected] wrote: > Author: xuhaihong > Date: Sun Jan 30 04:51:55 2011 > New Revision: 1065184 > > URL: http://svn.apache.org/viewvc?rev=1065184&view=rev > Log: > a. Add a lock for loadingConfiguration > b. Use fine-grained lock for getConfiguration > > Modified: > > geronimo/server/trunk/framework/modules/geronimo-kernel/src/main/java/org/apache/geronimo/kernel/config/SimpleConfigurationManager.java > > Modified: > geronimo/server/trunk/framework/modules/geronimo-kernel/src/main/java/org/apache/geronimo/kernel/config/SimpleConfigurationManager.java > URL: > http://svn.apache.org/viewvc/geronimo/server/trunk/framework/modules/geronimo-kernel/src/main/java/org/apache/geronimo/kernel/config/SimpleConfigurationManager.java?rev=1065184&r1=1065183&r2=1065184&view=diff > ============================================================================== > --- > geronimo/server/trunk/framework/modules/geronimo-kernel/src/main/java/org/apache/geronimo/kernel/config/SimpleConfigurationManager.java > (original) > +++ > geronimo/server/trunk/framework/modules/geronimo-kernel/src/main/java/org/apache/geronimo/kernel/config/SimpleConfigurationManager.java > Sun Jan 30 04:51:55 2011 > @@ -76,6 +76,7 @@ public class SimpleConfigurationManager > */ > private Configuration reloadingConfiguration; > > + private Object reloadingConfigurationLock = new Object(); > > public SimpleConfigurationManager(Collection<ConfigurationStore> stores, > ArtifactResolver artifactResolver, Collection<? extends Repository> > repositories, BundleContext bundleContext) { > this(stores, artifactResolver, repositories, > Collections.<DeploymentWatcher>emptySet(), bundleContext); > @@ -123,14 +124,18 @@ public class SimpleConfigurationManager > return false; > } > > - public synchronized boolean isLoaded(Artifact configId) { > + public boolean isLoaded(Artifact configId) { > if (!configId.isResolved()) { > throw new IllegalArgumentException("Artifact " + configId + " is > not fully resolved"); > } > - if (reloadingConfiguration != null && > reloadingConfiguration.getId().equals(configId)) { > - return true; > + synchronized (reloadingConfigurationLock) { > + if (reloadingConfiguration != null && > reloadingConfiguration.getId().equals(configId)) { > + return true; > + } > + } > + synchronized (this) { > + return configurationModel.isLoaded(configId); > } > - return configurationModel.isLoaded(configId); > } > > public synchronized boolean isRunning(Artifact configId) { > @@ -244,7 +249,7 @@ public class SimpleConfigurationManager > if (!artifact.isResolved()) { > throw new IllegalArgumentException("Artifact " + artifact + " is > not fully resolved"); > } > - synchronized (this) { > + synchronized (configurations) { > // if it is loaded, it is definitely a configuration > if (configurations.containsKey(artifact)) { > return true; > @@ -260,14 +265,18 @@ public class SimpleConfigurationManager > return false; > } > > - public synchronized Configuration getConfiguration(Artifact > configurationId) { > + public Configuration getConfiguration(Artifact configurationId) { > if (!configurationId.isResolved()) { > throw new IllegalArgumentException("Artifact " + configurationId > + " is not fully resolved"); > } > - if (reloadingConfiguration != null && > reloadingConfiguration.getId().equals(configurationId)) { > - return reloadingConfiguration; > + synchronized (reloadingConfigurationLock) { > + if (reloadingConfiguration != null && > reloadingConfiguration.getId().equals(configurationId)) { > + return reloadingConfiguration; > + } > + } > + synchronized (configurations) { > + return configurations.get(configurationId); > } > - return configurations.get(configurationId); > } > > public Bundle getBundle(Artifact id) { > @@ -308,17 +317,12 @@ public class SimpleConfigurationManager > } > > // load the configuration > -// LifecycleResults results = loadConfiguration(configurationData, > monitor); > LifecycleResults results = new LifecycleResults(); > - if (!configurations.containsKey(configurationId)) { > - configurationModel.addConfiguration(configurationId, > - Collections.<Artifact>emptySet(), > - Collections.<Artifact>emptySet()); > -// configurations.put(configurationId, configuration); > - > -// throw new NoSuchConfigException(configurationId, "not loaded"); > + synchronized (configurations) { > + if (!configurations.containsKey(configurationId)) { > + configurationModel.addConfiguration(configurationId, > Collections.<Artifact> emptySet(), Collections.<Artifact> emptySet()); > + } > } > -// addNewConfigurationToModel(configurations.get(configurationId)); > load(configurationId); > return results; > } > @@ -1133,7 +1137,9 @@ public class SimpleConfigurationManager > > if (configurationId.equals(newConfigurationId)) { > newConfiguration = configuration; > - reloadingConfiguration = configuration; > + synchronized (reloadingConfigurationLock) { > + reloadingConfiguration = configuration; > + } > } else { > loadedParents.put(configurationId, configuration); > } > @@ -1239,7 +1245,9 @@ public class SimpleConfigurationManager > > existingUnloadedConfiguration.getResolvedParentIds(), > Collections.<Artifact, Configuration>emptyMap() > ); > - reloadingConfiguration = configuration; > + synchronized (reloadingConfigurationLock) { > + reloadingConfiguration = configuration; > + } > // if the configuration was started before restart it > if (started.contains(existingConfigurationId)) { > start(configuration); > @@ -1271,7 +1279,9 @@ public class SimpleConfigurationManager > throw new LifecycleException("reload", > newConfigurationId, results); > } > } finally { > - reloadingConfiguration = null; > + synchronized (reloadingConfigurationLock) { > + reloadingConfiguration = null; > + } > } > } > > @@ -1307,7 +1317,9 @@ public class SimpleConfigurationManager > Collections.<Artifact, > Configuration>emptyMap() > ); > - reloadingConfiguration = configuration; > + synchronized (reloadingConfigurationLock) { > + reloadingConfiguration = configuration; > + } > monitor.succeeded(configurationId); > > // if the configuration was started before restart it > @@ -1357,7 +1369,9 @@ public class SimpleConfigurationManager > skip.add(failedId); > } > } finally { > - reloadingConfiguration = null; > + synchronized (reloadingConfigurationLock) { > + reloadingConfiguration = null; > + } > } > } > > >
