[
https://issues.apache.org/jira/browse/CONFIGURATION-712?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16615222#comment-16615222
]
Oliver Heger commented on CONFIGURATION-712:
--------------------------------------------
Thanks for the patch!
It looks good, and I also like the idea with the new refresh() method. The only
thing I am not sure about is the change on the isReloadingRequired() method. Is
this really necessary?
If I understand correctly, the changed method now expects that refresh() has
been called before. This would change the behavior for users that create an
instance of FileHandlerReloadingDetector directly without the factory. If the
method was not changed, it would now still work correctly with the updated
factory, wouldn't it?
> FileHandlerReloadingDetector Does Not Correctly Initialize File Last Modified
> -----------------------------------------------------------------------------
>
> Key: CONFIGURATION-712
> URL: https://issues.apache.org/jira/browse/CONFIGURATION-712
> Project: Commons Configuration
> Issue Type: Bug
> Components: File reloading
> Affects Versions: 2.3
> Reporter: Rolland Hobbie
> Priority: Major
> Attachments: CONFIGURATION-712-InitializeLastModified.patch,
> ReloadingFileBasedConfigurationBuilderExampleTest.java,
> ReloadingFileBasedConfigurationBuilderTest.java
>
> Original Estimate: 24h
> Remaining Estimate: 24h
>
> [FileHandlerReloadingDetector|https://commons.apache.org/proper/commons-configuration/apidocs/src-html/org/apache/commons/configuration2/reloading/FileHandlerReloadingDetector.html]
> declares the following method:
>
> {code:java}
> 150 @Override
> 151 public boolean isReloadingRequired()
> 152 {
> 153 long now = System.currentTimeMillis();
> 154 if (now >= lastChecked + getRefreshDelay())
> 155 {
> 156 lastChecked = now;
> 157
> 158 long modified = getLastModificationDate();
> 159 if (modified > 0)
> 160 {
> 161 if (lastModified == 0)
> 162 {
> 163 // initialization
> 164 updateLastModified(modified);
> 165 }
> 166 else
> 167 {
> 168 if (modified != lastModified)
> 169 {
> 170 return true;
> 171 }
> 172 }
> 173 }
> 174 }
> 175
> 176 return false;
> 177 }
> {code}
>
> During initialization of FileHandlerReloadingDetector, lastModified is never
> instantiated, so the first time isReloadingRequired() is invoked lastModified
> will be 0.
>
> This results in two issues:
>
> Test #1
> * Scenario Steps
> ## Initialize ReloadingFileBasedConfigurationBuilder without invoking
> builder.getConfiguration()
> ## Wait longer than refreshDelay
> ## Invoke builder.getReloadingController().checkForReloading(null) to notify
> the FileHandlerReloadingDetector to check for reload
> ## Invoke bulider.getConfiguration()
> ## Update the properties file
> ## Wait longer than refreshDelay
> ## Invoke builder.getReloadingController().checkForReloading(null) to notify
> the FileHandlerReloadingDetector to check for reload
> ## Invoke bulider.getConfiguration()
> * Result - the Configuration does not have the updated value.
>
> Test #2
> * Scenario Steps
> ## Initialize ReloadingFileBasedConfigurationBuilder without invoking
> builder.getConfiguration()
> ## Invoke bulider.getConfiguration()
> ## Update the properties file
> ## Wait longer than refreshDelay
> ## Invoke builder.getReloadingController().checkForReloading(null) to notify
> the FileHandlerReloadingDetector to check for reload
> ## Invoke bulider.getConfiguration()
> ## Update the properties file
> ## Wait longer than refreshDelay
> ## Invoke builder.getReloadingController().checkForReloading(null) to notify
> the FileHandlerReloadingDetector to check for reload
> ## Invoke bulider.getConfiguration()
> * Result - For the first two invocations, the Configuration is not updated.
> One the third invocation of builder.getConfiguration() the property is
> updated to the new value.
>
> As potential solution, the constructor of FileHandlerReloadingDetector should
> either call isReloadingRequired() or
> updateLastModified(getLastModificationDate()) to initialize the lastModified
> instance variable to the file current lastModified value.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
