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

[Jie Kang]

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

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.


Regards

> J.
> 

-- 

Jie Kang

OpenJDK Team - Software Engineering Intern