[rfc] [icedtea-web] make PAthsAndFilles to follow seted paths IV - bubble to CacheLruWrapper
Jie Kang
jkang at redhat.com
Thu Apr 9 19:25:39 UTC 2015
Hello,
Tiny nits below:
+ //the InfrastructureFileDescriptor was set to different location, move to it
+ cachedRecentlyUsedPropertiesFile.lock();
+ cachedRecentlyUsedPropertiesFile.store();
+ cachedRecentlyUsedPropertiesFile.unlock();
+ /// hopefully above do its job
lock() blocks indefinitely until the lock is available. If someone else takes the lock and doesn't release it, this can block forever. I suggest using tryLock() instead since the store() is nice but not necessary. Something like:
if (cachedRecentlyUsedPropertiesFile.tryLock() {
[do stuff...]
}
Otherwise you'll have to add more complicated code to prevent blocking indefinitely.
The comment "/// hopefully above do its job" isn't useful. Could you remove it?
+ * Remove garbage from paths.
+ *
+ * Currently this methods unify all multiple occurrences of separators
+ * to single one. Eg /path/to//file will become /path/to/file.
I would reword the comment "Remove garbage from paths" to something like "Removes extra file separators from paths". It's much more descriptive that way.
All the other changes look fine. I ran reproducers and unit tests before/after this patch and didn't see anything out of the ordinary (but our tests don't cover everything). Please double-check to see if anything is broken. Thanks for your work!
Regards,
----- Original Message -----
> On 04/08/2015 09:02 PM, Jie Kang wrote:
> > Hello,
> >
> > Good work!
> >
> > Nits below:
> >
> >
> > For this patch, the class DeploymentPropetiesModifier needs to be fixed to
> > use the new InfrastructureFileDescriptor. It currently doesn't compile.
> > Also if you could: s/Propeties/Properties
> >
> >
> > diff -r 85505d8c9f3c
> > tests/netx/unit/net/sourceforge/jnlp/controlpanel/CommandLineTest.java
> > ---
> > a/tests/netx/unit/net/sourceforge/jnlp/controlpanel/CommandLineTest.java
> > Thu Apr 02 21:28:10 2015 +0200
> > +++
> > b/tests/netx/unit/net/sourceforge/jnlp/controlpanel/CommandLineTest.java
> > Thu Apr 02 21:57:28 2015 +0200
> > @@ -30,6 +30,7 @@
> >
> > import net.sourceforge.jnlp.OptionsDefinitions;
> > import net.sourceforge.jnlp.config.PathsAndFiles;
> > +import net.sourceforge.jnlp.util.logging.NoStdOutErrTest;
> > import net.sourceforge.jnlp.util.logging.OutputController;
> > import net.sourceforge.jnlp.util.optionparser.OptionParser;
> > import net.sourceforge.jnlp.util.optionparser.UnevenParameterException;
> > @@ -37,7 +38,7 @@
> > import org.junit.BeforeClass;
> > import org.junit.Test;
> >
> > -public class CommandLineTest {
> > +public class CommandLineTest extends NoStdOutErrTest{
> >
> > public static final int ERROR = 1;
> > public static final int SUCCESS = 0;
> > @@ -91,7 +92,7 @@
> > ByteArrayOutputStream outStream = getOutputControllerStream();
> >
> > String[] args = {
> > - "set", "unknown", "ALLOW_UNSIGNED"
> > + "set", "unknown", "does_not_metter"
> >
> > s/metter/matter/
> >
> > };
> > OptionParser optionParser = new OptionParser(args,
> > OptionsDefinitions.getItwsettingsCommands());
> > CommandLine commandLine = new CommandLine(optionParser);
> > @@ -150,7 +151,7 @@
> > @Test
> > public void testSetPropertyWithIncorrectValue() throws IOException {
> > String[] args = {
> > - "set", "deployment.security.level",
> > "ALLOW_ONLY_SAFE_APPLETS"
> > + "set", "deployment.security.level",
> > "INTENTIONALLY_INCORRECT"
> > };
> >
> > Is this changeset for CommandTest at all related to your patch? If not,
> > this should be a separate patch.
>
> Yes it is. They were failing for me during work on this patch. And it was
> pretty confusing the
> original naming.
> >
> >
> > diff -r 85505d8c9f3c
> > tests/netx/unit/net/sourceforge/jnlp/security/KeyStoresTest.java
> > --- a/tests/netx/unit/net/sourceforge/jnlp/security/KeyStoresTest.java Thu
> > Apr 02 21:28:10 2015 +0200
> > +++ b/tests/netx/unit/net/sourceforge/jnlp/security/KeyStoresTest.java Thu
> > Apr 02 21:57:28 2015 +0200
> > @@ -37,6 +37,7 @@
> > package net.sourceforge.jnlp.security;
> >
> > import java.security.Permission;
> > +import net.sourceforge.jnlp.config.InfrastructureFileDescriptor;
> > import net.sourceforge.jnlp.config.PathsAndFiles;
> > import org.junit.AfterClass;
> > import org.junit.Assert;
> > @@ -62,7 +63,7 @@
> > //TODO once setConfig is removed, ensure SM is enforced also from
> > PathsAndFiles
> >
> > Has this TODO been completed? Or is it still valid?
>
> Nope - removed. The SM is called a bit differently, but even more validly
> then before.
> >
> >
> > + //he underlying InfrastructureFileDescriptor is still poinitng
> > to the same file, use current proeprties file
> >
> > s/he/The
> > s/poinitng/pointing
> > s/proeprties/properties
>
> fixed.
>
> >
> > + //the InfrastructureFileDescriptor was set to different
> > location, move to it
> > + //is there something needed to close previous instance?
> >
> > I wouldn't put a question in the comment unless there is a TODO or FIXME.
> > Otherwise it's confusing to other developers (should they be looking at
> > fixing this or not?)
>
> fixed
> >
> > I don't think anything else is needed. Saving the properties to file using
> > store() is nice to do, but I wouldn't say it's necessary.
> >
> > Also, the word 'move' isn't too correct as nothing is being moved. The
> > variable is just pointing to a new location. s/move to it/use the new
> > location
> >
> > + cachedPropertiesFile.store();
> > + cachedPropertiesFile.unlock();
>
> added lock before store.
> >
> > In order to store you must have the lock. So you'd need to lock() or
> > tryLock() before calling store().
> >
> > Also, the variable cachedPropertiesFile refers to the RecentlyUsed file
> > that is of type PropertiesFile. I'd rename the variable to
> > cachedRecentlyUsedFile, or something like that.
>
> fixed
> >
> > + //above were just shotted here
> >
> > I don't understand this comment. Can you reword it?
>
> removed
> >
> > + * @return true if proeprtieswere sucesfullty stored, fasle otherwise
> >
> > s/proeprtieswere/properties were
> > s/sucesfullty/successfully
> > s/fasle/false
>
> fixed O:(
> >
> > + PropertiesFile props = getRecentlyUsedPropertiesFile();
> > + if (!props.containsKey(oldKey)) return false;
> >
> > Please don't use one line if statements.
>
> it was not me! I run only refactoring on field ;) Anyway fixed. It is always
> got to get rid of them.
> >
> > + rStr =lruHandler.getCacheDir().getFullPath()+
> > rStr.substring(0, rStr.indexOf(File.separatorChar, 1));
> >
> > Please fix the spacing near the '=' character as well.
>
> done
> >
> > + //simple constructor to allow testing instances base don overrides
> >
> > s/base don/based on
>
> done
> >
> > + //simple constructor to allow testing instances base don overrides
> > + protected InfrastructureFileDescriptor() {
> > + this("nope", "nope", "nope", "nope");
> > + }
> >
> > I'd use empty string "" instead of "nope". The "nope" string doesn't look
> > as professional IMO.
>
> used undef rather.
> >
> > + final private DeploymentConfiguration config;
> >
> > s/final private/private final
> >
> > + @Override
> > + public String getFullPath() {
> > + return backend.getAbsolutePath();
> > + }
> > +
> > +
> > +
> > +
> > +
> >
> > Please remove the extra lines.
>
> done
>
> TY!
> >
> > Thanks for the patch!
> >
> >
> > Regards,
> >
> >
> >
> >
> > ----- Original Message -----
> >> On 04/02/2015 09:54 PM, Jiri Vanek wrote:
> >>> Adapted to head.
> >>
> >> forgot hg add
> >>>
> >>> However I have not checked removal of constructors properly... BUt Maybe
> >>> I
> >>> missed none. TY!
> >>>
> >>> J.
> >>>
> >>>
> >>> On 04/01/2015 04:34 PM, Jiri Vanek wrote:
> >>>> Seems like I forgot an attachment...
> >>>>
> >>>> On 03/31/2015 03:51 PM, Jiri Vanek wrote:
> >>>>> There are/were/reappeared two issues in LRUwrapper and CacheUtils
> >>>>>
> >>>>> First the file - however crated was as value,not as pointer. So
> >>>>> whatever
> >>>>> happened to original, had not bubbled to lruwrapper/utils.
> >>>>> Second - the methods were using getters on field, whcch could change
> >>>>> during the method invocation.
> >>>>>
> >>>>> This pathc should fix both of them.
> >>>>>
> >>>>> The main refactoring is
> >>>>> - public CacheLRUWrapper(final File recentlyUsed, final File
> >>>>> cacheDir)
> >>>>> {
> >>>>> - recentlyUsedPropertiesFile = new PropertiesFile(recentlyUsed);
> >>>>> + public CacheLRUWrapper(final InfrastructureFileDescriptor
> >>>>> recentlyUsed, final InfrastructureFileDescriptor cacheDir) {
> >>>>> + recentlyUsedPropertiesFile = recentlyUsed;
> >>>>>
> >>>>> Now the cache lru wrapper is handling the "pointers" to sources. So any
> >>>>> call to its "getFiles" get real value fromm PathsAndFiles
> >>>>> When any method is using its getFile it alwasy save current copy, co
> >>>>> during runtime, it will not be affected by possible change.
> >>>>>
> >>>>> The exception is
> >>>>>
> >>>>> - public PropertiesFile getRecentlyUsedPropertiesFile() {
> >>>>> - return recentlyUsedPropertiesFile;
> >>>>> + PropertiesFile getRecentlyUsedPropertiesFile() {
> >>>>> + if (cachedPropertiesFile == null) {
> >>>>> + //no properties file yet, create it
> >>>>> + cachedPropertiesFile = new
> >>>>> PropertiesFile(recentlyUsedPropertiesFile.getFile());
> >>>>> + return cachedPropertiesFile;
> >>>>> + }
> >>>>> + if
> >>>>> (recentlyUsedPropertiesFile.getFile().equals(cachedPropertiesFile.getStoreFile())){
> >>>>> + //he underlying InfrastructureFileDescriptor is still
> >>>>> poinitng to the same file, use current proeprties file
> >>>>> + return cachedPropertiesFile;
> >>>>> + } else {
> >>>>> + //the InfrastructureFileDescriptor was set to different
> >>>>> location, move to it
> >>>>> + //is there something needed to close previous instance?
> >>>>> + cachedPropertiesFile.store();
> >>>>> + cachedPropertiesFile.unlock();
> >>>>> + //above were just shotted here
> >>>>> + cachedPropertiesFile = new
> >>>>> PropertiesFile(recentlyUsedPropertiesFile.getFile());
> >>>>> + return cachedPropertiesFile;
> >>>>> + }
> >>>>> +
> >>>>>
> >>>>>
> >>>>> When something change value path to properties file *and* call
> >>>>> getRecentlyUsedPropertiesFile then the file will store and unlock.
> >>>>> (note
> >>>>> - I dont know for sure if those two are needed, but afaik yes). So
> >>>>> actions upo previous Prop.File may misbehave.
> >>>>> However .. The setter to the RECENTLY_USED_FILE and KEY_USER_CACHE_DIR
> >>>>> is
> >>>>> used *only* in tests (and should never be used in ITW itself)
> >>>>> Afaik - whole those will work if you run javaws (keep running) change
> >>>>> proeprties, run second javaws. I noticed no issues. Everything was
> >>>>> correctly cached twice
> >>>>>
> >>>>>
> >>>>> There is one aditional refactoring in this patch - sorry for that -
> >>>>> InfrastructureFileDescriptor moved from inner class to outer class.
> >>>>> With
> >>>>> one change -
> >>>>> + //simple constructor to allow testing instances base don overrides
> >>>>> + protected InfrastructureFileDescriptor() {
> >>>>> + this("nope", "nope", "nope", "nope");
> >>>>> + }
> >>>>>
> >>>>>
> >>>>> At the end, I consider this 4 pathces + lrucaches singleton +
> >>>>> PAthsAndFiles as reallly good thing which happend to itw.
> >>>>>
> >>>>>
> >>>>> Jie - Is this enough for your "[rfc][icedtea-web] Use temporary cache
> >>>>> in
> >>>>> *" or is there something more to be done?
> >>>>>
> >>>>>
> >>>>> J.
> >>>>
> >>>
> >>
> >>
> >
>
>
--
Jie Kang
OpenJDK Team - Software Engineering Intern
More information about the distro-pkg-dev
mailing list