commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Oliver Heger (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CONFIGURATION-712) FileHandlerReloadingDetector Does Not Correctly Initialize File Last Modified
Date Fri, 14 Sep 2018 18:38:00 GMT

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

Mime
View raw message