geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Jencks <david_jen...@yahoo.com>
Subject Re: svn commit: r1065184 - /geronimo/server/trunk/framework/modules/geronimo-kernel/src/main/java/org/apache/geronimo/kernel/config/SimpleConfigurationManager.java
Date Sun, 30 Jan 2011 07:36:07 GMT
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;
> +                }
>             }
>         }
> 
> 
> 


Mime
View raw message