Deadlock during refresh properties

• Key: CONFIGURATION-344
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Critical
• Assignee: Oliver Heger
• Reporter: Pavel Savara
• Component/s: Events & Notifications, File reloading
• Affects Version/s: 1.5
• Fix Version/s: 1.6

Description

Hi
Commons configurations get itself stuck in deadlock when refreshing
properties using Managed reloading strategy. It seems to me it get stuck
because of fireEvent in reload method. Another access grabs lock on
synchronized (getNodeCombiner()) when trying to rebuild but Combined
configuration is one of the listeners for event es well and it gets
stuck when processing invalidate. Can anyone suggest quick fix please?
Relevant information follows.
Thanks
Pavel

Configuration:

<configuration> 
  <override>
    <system/>    
    <properties fileName="gsxweb.properties" throwExceptionOnMissing="false"
       config-name="gsxweb" config-optional="false" listDelimiter="|">
       <reloadingStrategy config-class="org.apache.commons.configuration.reloading.ManagedReloadingStrategy"/>      
    </properties>    
  </override> 
</configuration>

Our Reload code:

int ln = combinedConfiguration.getNumberOfConfigurations();
        int reloaded = 0;
        for (int i = 0; i < ln; i++) {
            Configuration conf = combinedConfiguration.getConfiguration(i);
            if (conf instanceof PropertiesConfiguration) {
                ManagedReloadingStrategy strat = null;
                ReloadingStrategy strategy = ((PropertiesConfiguration) conf).getReloadingStrategy();
                //refresh if managed strategy
                if (strategy instanceof ManagedReloadingStrategy) {
                    ((ManagedReloadingStrategy) strategy).refresh();
                //reload if file changed strategy    
                } else if (strategy instanceof FileChangedReloadingStrategy) {                    
                    ((PropertiesConfiguration) conf).reload();
                }
                reloaded++;
            }
        }

Stack trace of deadlock threads
Name: http-10980-1
State: BLOCKED on
org.apache.commons.configuration.tree.OverrideCombiner@8511bb owned by:
http-10980-6
Total blocked: 154  Total waited: 2

Stack trace: 
org.apache.commons.configuration.CombinedConfiguration.invalidate(CombinedConfiguration.java:474)
org.apache.commons.configuration.CombinedConfiguration.configurationChanged(CombinedConfiguration.java:488)
org.apache.commons.configuration.event.EventSource.fireEvent(EventSource.java:249)
org.apache.commons.configuration.AbstractFileConfiguration.fireEvent(AbstractFileConfiguration.java:911)
org.apache.commons.configuration.AbstractFileConfiguration.reload(AbstractFileConfiguration.java:828)
   - locked java.lang.Object@127e34c
org.apache.commons.configuration.AbstractFileConfiguration.isEmpty(AbstractFileConfiguration.java:927)
org.apache.commons.configuration.reloading.ManagedReloadingStrategy.refresh(ManagedReloadingStrategy.java:91)
com.gsx.properties.PropertyProviderImpl.reset(PropertyProviderImpl.java:203)
   - locked java.lang.Class@109bcda
org.apache.jsp.test.testPropertyProvider_jsp._jspService(testPropertyProvider_jsp.java:60)
org.apache.jasper.runtime.HttpJspBase.service(HttpJspBase.java:70)
javax.servlet.http.HttpServlet.service(HttpServlet.java:717)
org.apache.jasper.servlet.JspServletWrapper.service(JspServletWrapper.java:374)
org.apache.jasper.servlet.JspServlet.serviceJspFile(JspServlet.java:342)
org.apache.jasper.servlet.JspServlet.service(JspServlet.java:267)
javax.servlet.http.HttpServlet.service(HttpServlet.java:717)
org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:290)


Name: http-10980-6
State: BLOCKED on java.lang.Object@127e34c owned by: http-10980-1
Total blocked: 115  Total waited: 2

Stack trace: 
org.apache.commons.configuration.AbstractFileConfiguration.reload(AbstractFileConfiguration.java:814)
org.apache.commons.configuration.AbstractFileConfiguration.getKeys(AbstractFileConfiguration.java:939)
org.apache.commons.configuration.ConfigurationUtils.copy(ConfigurationUtils.java:139)
org.apache.commons.configuration.ConfigurationUtils.convertToHierarchical(ConfigurationUtils.java:199)
org.apache.commons.configuration.CombinedConfiguration
$ConfigData.getTransformedRoot(CombinedConfiguration.java:794)
org.apache.commons.configuration.CombinedConfiguration.constructCombinedNode(CombinedConfiguration.java:653)
org.apache.commons.configuration.CombinedConfiguration.getRootNode(CombinedConfiguration.java:504)
   - locked
org.apache.commons.configuration.tree.OverrideCombiner@8511bb
org.apache.commons.configuration.HierarchicalConfiguration.fetchNodeList(HierarchicalConfiguration.java:925)
org.apache.commons.configuration.HierarchicalConfiguration.getProperty(HierarchicalConfiguration.java:327)
org.apache.commons.configuration.CombinedConfiguration.getProperty(CombinedConfiguration.java:578)
org.apache.commons.configuration.AbstractConfiguration.resolveContainerStore(AbstractConfiguration.java:1155)
org.apache.commons.configuration.AbstractConfiguration.getString(AbstractConfiguration.java:1034)
org.apache.jsp.test.testPropertyProvider_jsp._jspService(testPropertyProvider_jsp.java:69)
org.apache.jasper.runtime.HttpJspBase.service(HttpJspBase.java:70)
javax.servlet.http.HttpServlet.service(HttpServlet.java:717)
org.apache.jasper.servlet.JspServletWrapper.service(JspServletWrapper.java:374)
org.apache.jasper.servlet.JspServlet.serviceJspFile(JspServlet.java:342)
org.apache.jasper.servlet.JspServlet.service(JspServlet.java:267)
javax.servlet.http.HttpServlet.service(HttpServlet.java:717)

Comments

[Pavel Savara]

Here is my quick dirty fix,
Because getNodeCombiner() is public method i can grab lock in my code first before try to do reloading.

synchronized (combinedConfiguration.getNodeCombiner()){
//do the reloading
}

It means locks are grabbed always in the same order so deadlock can't happen.

Best
Pavel

[Oliver Heger]

This is indeed a classic deadlock scenario. I can reproduce the problem with a unit test.

While testing I found another problem related to reloading strategies. I created CONFIGURATION-347 for this issue.

[Oliver Heger]

A fix was applied to trunk in revision 712401.

So far, accessing the root node of the combined configuration was synchronized because it had to be re-constructed if something had changed. This fix removes this synchronization and - to ensure proper visibility - adds the volatile modifier to the root node field. If now multiple threads want to access the combined configuration, there may be a race condition causing the root node to be constructed multiple times. However, this does not impact the results of queries or have other undesired side effects.

When testing I found out that reloading operations are indeed problematic when multiple threads are involved. The unit test case uses a synthetic reloading strategy that forces a reload on every access. AbstractFileConfiguration overrides all access methods to ensure that reload() is invoked as in the following example:

public Object getProperty(String key)
    {
        reload();
        return super.getProperty(key);
    }

If there are multiple concurrent reloads or property accesses in sequence - as in the test I created -, the following race condition can happen:

• One thread executes reload() and then is going to enter getProperty().
• Another thread wants to read a property and enters reload(). Because reload() is guarded by a lock, the thread has to wait.
• As soon as the first thread leaves reload() the second can proceed and initiates the reload operation.
• While trying to read a property the first thread sees an invalid state because a reload operation is in progress.

To get my test running I changed the implementation of getProperty() in the following way:

public Object getProperty(String key)
    {
        synchronized (reloadLock)
        {
            reload();
            return super.getProperty(key);
        }
    }

So the whole property read operation is now guarded by the lock. To be on the safe side this change would have to be applied for the other methods, too. But I think, the architecture of the reloading implementation should better be re-worked in the next major release. Note that it is very unlikely that multiple reloads happen in short intervals because the available reloading strategies have a refresh delay.