You are viewing a plain text version of this content. The canonical link for it is here.
Posted to scm@geronimo.apache.org by xu...@apache.org on 2011/01/30 05:51:55 UTC
svn commit: r1065184 -
/geronimo/server/trunk/framework/modules/geronimo-kernel/src/main/java/org/apache/geronimo/kernel/config/SimpleConfigurationManager.java
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;
+ }
}
}
Re: svn commit: r1065184 - /geronimo/server/trunk/framework/modules/geronimo-kernel/src/main/java/org/apache/geronimo/kernel/config/SimpleConfigurationManager.java
Posted by David Jencks <da...@yahoo.com>.
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, xuhaihong@apache.org 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;
> + }
> }
> }
>
>
>