[jira] [Comment Edited] (CONFIGURATION-712) FileHandlerReloadingDetector Does Not Correctly Initialize File Last Modified

["Rolland Hobbie (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CONFIGURATION-712?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16616600#comment-16616600 ] 

Rolland Hobbie edited comment on CONFIGURATION-712 at 9/16/18 5:39 AM:
-----------------------------------------------------------------------

I have attached the updated patch. I reverted the change in the isReloadingRequired() method of FileHandlerReloadingDetector. I also reverted the two test methods that were changed (leaving these methods unchanged to validate the original functionality), and added two new methods to test the new functionality with invoking the refresh method after instantiation.

 

Since you asked about documentation, I think it would be worth adding a note to FileHandlerReloadingDetector along the lines of:
{quote}There is a cycle between the instantiation of FileHandlerReloadingDetector and the first invocation of isReloadingRequired() where no file changes are detected. The first file change detected by default is after the first invocation of isReloadingRequired(), and because FileHandlerReloadingDetector is recreated when the configuration is reset, this "unobserved cycle" is repeated after every reload of a configuration. To eliminate this "unobserved cycle", the refresh() method can be invoked after instantiation to force the FileHandlerReloadingDetector to load the file's current last modified date, which is used in future calls of isReloadingRequired() to determine if the file has changed.
{quote}


was (Author: rllndhbb):
I have attached the updated patch. I reverted the change in the isReloadingRequired() method of FileHandlerReloadingDetector. I also reverted the two test methods that were changed (leaving these methods unchanged to validate the original functionality), and added two new methods to test the new functionality with invoking the refresh method after instantiation.

 

Since you asked about documentation, I think it would be worth adding note to FileHandlerReloadingDetector along the lines of:

bq. There is a cycle between the instantiation of FileHandlerReloadingDetector and the first invocation of isReloadingRequired() where no file changes are detected. The first file change detected by default is after the first invocation of isReloadingRequired(), and because FileHandlerReloadingDetector is recreated when the configuration is reset, this "unobserved cycle" is repeated after every reload of a configuration. To eliminate this "unobserved cycle", the refresh() method can be invoked after instantiation to force the FileHandlerReloadingDetector to load the file's current last modified date, which is used in future calls of isReloadingRequired() to determine if the file has changed.

> 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)