[rfc] [icedtea-web] make PAthsAndFilles to follow seted paths IV - bubble to CacheLruWrapper
Jiri Vanek
jvanek at redhat.com
Thu Apr 9 10:04:23 UTC 2015
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.
>>>>
>>>
>>
>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: madeLRUcahceWrapperToUseInfrastructureFileDescriptorDirectly4.patch
Type: text/x-patch
Size: 40934 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20150409/c926be85/madeLRUcahceWrapperToUseInfrastructureFileDescriptorDirectly4-0001.patch>
More information about the distro-pkg-dev
mailing list