[rfc] [icedtea-web] make PAthsAndFilles to follow seted paths IV - bubble to CacheLruWrapper

Jiri Vanek jvanek at redhat.com
Fri Apr 10 10:48:16 UTC 2015


On 04/09/2015 09:25 PM, Jie Kang wrote:
> 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...]
> }


Nice catch!
fixed!
>
> 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.
>
I would much rather keep it as it is. It is garbage. And although now the moethod only fixes (/)+ to 
/ in fact it can deal with various garbage.
>
> 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!
>
>
tyvm for review. pushed.

J.
> 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.
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>



More information about the distro-pkg-dev mailing list