[rfc][icedtea-web] fixing CacheReproducerTest and improving VersionedJarTest

[Jiri Vanek]

On 03/04/2015 04:01 PM, Jie Kang wrote:
>
>
> ----- Original Message -----
>> snip
>>>
>>> +    private final PropertiesFile recentlyUsedPropertiesFile;
>>> +    private final File cacheDir;
>>> +    private final File recentlyUsedFile;
>>>
>>> +    /**
>>> +     * @return the recentlyUsedFile
>>> +     */
>>> +    public File getRecentlyUsedFile() {
>>> +        return recentlyUsedFile;
>>> +    }
>>>
>>> Instead of keeping recentlyUsedFile in memory, can you change it to:
>>>
>>> +    public File getRecentlyUsedFile() {
>>> +        return recentlyUsedPropertiesFile.getStoreFile();
>>> +    }
>>>
>>> Then you can remove that variable.
>>
>> As    you wish, done.
>>>
>>>
>>> -        if (cacheOrder.containsKey(key)) {
>>> +        if (getRecentlyUsedPropertiesFile().containsKey(key)) {
>>>
>>> AFAICT this code is inside CacheLRUWrapper so it should have access to
>>> 'recentlyUsedPropertiesFile'. It shouldn't use the getter if it doesn't
>>> need to, no?
>>>
>>> A lot of the code changes are like this. Can you change them all back?
>>
>> With the getter you wished no. And I vote for keeping the getter rather
>>>
>>>
>>>
>>>
>>> Also:
>>>
>>> +    public CacheLRUWrapper() {
>>> +     this(PathsAndFiles.RECENTLY_USED_FILE.getFile());
>>> +    }
>>>
>>> In terms of the constructor, the dependencies are opposite.
>>>
>>> Right now, you get recently_used file, and then get cacheDir off it.
>>>
>>> Instead, it should be get cacheDir, and then get recently_used file off it.
>>>
>>> E.g.
>>>
>>> RecentlyUsedFile is always inside the Cache.  (this is correct)
>>>
>>> as opposed to
>>>
>>> Cache is always parent file of RecentlyUsedFile. (In most cases this ends
>>> up being the same thing, but it's not as correct)
>>
>> Right. I removed misleading single-file constructor.
>>>
>>>
>>>
>>> The default constructor usage ends up breaking things when you use it in
>>> CacheUtil.
>>>
>>>       private static final String setCacheDir =
>>>       JNLPRuntime.getConfiguration().getProperty(DeploymentConfiguration.KEY_USER_CACHE_DIR);
>>>       private static final String cacheDir = new File(setCacheDir != null ?
>>>       setCacheDir : System.getProperty("java.io.tmpdir")).getPath(); // Do
>>>       this with file to standardize it.
>>>       private static final CacheLRUWrapper lruHandler =
>>>       CacheLRUWrapper.getInstance();
>>>
>>>    You need to also add changes to CacheUtil to use lruHandler's cache
>>>    directory.
>>>
>>>    At the moment, CacheUtil.cacheDir != lruHandler.getCacheDir() when
>>>    someone sets DeploymentConfiguration.KEY_USER_CACHE_DIR to non-default.
>>
>> Yes. Fixed.
>>>
>>>
>>> This patch is broken here and needs to be fixed.
>>>
>>>
>>> Why would you push this without a +1 from anyone? You also seem to be doing
>>> this with a number of other patches recently. If you need a review, ping
>>> someone.
>>
>> :( Rush is an eneymy. I need to calm bit down with all those failures...
>>
>>
>> See the patch for this commit.
>
> Hello,
>
>
> -    private static final CacheLRUWrapper lruHandler = CacheLRUWrapper.getInstance();
>
> I feel like keeping a static instance here is still okay... Does it cause any problems?

Unless you insists, I will go with the field removed.

>
> When the code inside CacheUtil calls: CacheLRUWrapper.getInstance() : it's receiving the same static instance anyways. So you should be able to just call it once and keep it for future use without any problems.
>
>
> Without the static instance you might want to move some code around to be more clean.
>
> E.g:
>
> @@ -145,12 +143,13 @@
>               return false;
>           }
>
> -        File cacheDir = new File(CacheUtil.cacheDir);
> +        File cacheDir = CacheLRUWrapper.getInstance().getCacheDir();
>           if (!(cacheDir.isDirectory())) {
>               return false;
>           }
>
>           OutputController.getLogger().log(OutputController.Level.ERROR_DEBUG, "Clearing cache directory: " + cacheDir);
> +        CacheLRUWrapper lruHandler = CacheLRUWrapper.getInstance();
>
> You should get the instance once before the line "File cacheDir = CacheLRUWrapper.getInstance().getCacheDir();", and reuse it throughout the method. This applies to a lot of the methods now.
>
>
> The rest looks good.
>
> The tests are all passing for me with this patch applied. For some reason though, I feel like there are still some hidden bugs in the caching system :\ I'll look into it.
>
>
Yes. The caching system had a lot of bugs in past. Mostl caused by various duplicated providers.

Ok to push  now?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: removeDuplicateDefinitionsInCacheUtilsAndUseDefaultsFromLruWrapper2.patch
Type: text/x-patch
Size: 6682 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20150304/14beb513/removeDuplicateDefinitionsInCacheUtilsAndUseDefaultsFromLruWrapper2.patch>