[
https://issues.apache.org/jira/browse/CONFIGURATION-390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12759942#action_12759942
]
Oliver Heger commented on CONFIGURATION-390:
--------------------------------------------
Okay, using the reloading lock for all methods that might be affected is
certainly the way to go (I was only too lazy to do this ;-) But to be
consistent, analogous changes should be applied to
{{AbstractFileConfiguration}}: Here we have the same pattern that methods first
call {{reload()}} and then do something with the data, which might already be
invalid due to another {{reload()}} operation.
Just an idea: Would it make sense to have a method like {{syncReload(Accessor
acc)}} in {{AbstractFileConfiguration}} like the following:
{code}
protected interface Accessor
{
Object access(FileConfiguration config);
}
protected Object syncReload(Accessor acc)
{
synchronized(reloadLock)
{
reload();
return acc.access(this);
}
}
{code}
Maybe this would reduce the clutter in the affected methods a bit and allow a
more closure-oriented programming.
Now to {{CombinedConfiguration}}: AFAICT the problem lies in the
{{getTransformedRoot()}} method of the helper class {{ConfigData}}. We have
here the following code:
{code}
// Copy data of the root node to the new path
rootNode = ConfigurationUtils.convertToHierarchical(getConfiguration(),
getConversionExpressionEngine()).getRootNode();
atParent.appendChildren(rootNode);
atParent.appendAttributes(rootNode);
{code}
To be save, this fragment has to be executed with the reload lock of the
processed configuration hold. A dirty hack could be to add the
{{getReloadLock()}} method to the {{FileConfiguration}} interface and check
whether the configuration implements this interface:
{code}
if(getConfiguration() instanceof FileConfiguration)
{
synchronized(((FileConfiguration) getConfiguration()).getReloadLock())
{
// Copy data of the root node to the new path
...
}
}
{code}
However, changing the {{FileConfiguration}} interface is a binary incompatible
change. So we might have to introduce an additional interface solely for this
purpose.
WDYT?
> AbstractHierarchicalFileConfiguration is not thread safe
> --------------------------------------------------------
>
> Key: CONFIGURATION-390
> URL: https://issues.apache.org/jira/browse/CONFIGURATION-390
> Project: Commons Configuration
> Issue Type: Bug
> Components: File reloading
> Affects Versions: 1.6
> Reporter: Emmanuel Bourg
> Fix For: 1.7
>
> Attachments: commons-configuration-390.patch, configtest.tar.gz,
> TestSuite.txt
>
>
> AbstractHierarchicalFileConfiguration doesn't implement the same locking
> mechanism found in AbstractFileConfiguration. The consequence is that getting
> a property while the configuration is being reloaded by another thread can
> return an invalid result.
> This can be demonstrated by changing testDeadlockWithReload() in
> TestCombinedConfiguration to use an XMLConfiguration instead of a
> PropertiesConfiguration.
> Here is a reduced test case:
> {code:java}
> public void testConcurrentGetAndReload() throws Exception
> {
> //final FileConfiguration config = new
> PropertiesConfiguration("test.properties");
> final FileConfiguration config = new XMLConfiguration("test.xml");
> config.setReloadingStrategy(new FileAlwaysReloadingStrategy());
> assertTrue("Property not found", config.getProperty("test.short") !=
> null);
> new Thread()
> {
> public void run()
> {
> for (int i = 0; i < 1000; i++)
> {
> config.reload();
> }
> }
> }.start();
>
> for (int i = 0; i < 1000; i++)
> {
> assertTrue("Property not found", config.getProperty("test.short") !=
> null); // failure here
> }
> }
> {code}
> The test doesn't always fail. It does about 50% of the time.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
