[RFC] Fix IndexOutOfBoundException because of corrupted entry in recently_used file
Jiri Vanek
jvanek at redhat.com
Fri Apr 6 03:01:19 PDT 2012
On 04/05/2012 03:30 PM, Thomas Meyer wrote:
> Am Donnerstag, den 05.04.2012, 14:59 +0200 schrieb Jiri Vanek:
>> On 04/04/2012 04:57 PM, Thomas Meyer wrote:
>>> Am Dienstag, den 03.04.2012, 16:50 +0200 schrieb Jiri Vanek:
>>>>> Thanx for test! Logic itself is good, but there are two mistakes which causes the test to be not-working.
>>>>> See notes inline for them.
...
>>
>> However to print stacktrace just in debug mode will be probably better idea.
>> Plus information text I have dared to add.
>
> fine for me.
I have moved the exception to debug mode.
>
>>>
...
>>> + * clean up possibly corrupted entries
>>> + */
>> It is interesting to see how many times is checkData called during single jar run.. I counted
>> 10executions.... But fixing this is definitely work for different patch.
>
> yeah, I also noticed this. maybe this can be reduced a bit. see below
> comment for getCacheFile(). but as the recently_used file could be
> corrupted by another jvm process (etc.), we need to check this on every
> load.
>
Fair enough
>>> + if(checkData())
>>> - lruHandler.store();
>> This scares me - why it have begin to be removed?
>
> in getCacheFile() method this happens:
>
> 1.) call load() -> this must happen, because of concurrent update by
> other jvm processes.
> 2.) checks if entry exists
> 2a.) when yes -> return the cacheFile entry
> 2b.) when no -> call makeNewCacheFile() -> will call load() and
> store().
> 3.) call store() again -> so this one can be omitted as the store will
> happen in makeNewCacheFile(), in already existing entry case, no store
> is needed.
> 4.) return cacheFile entry
>
> so here a superfluous store() is removed, as the store() will happen in
Thanx. Ok then.
> makeNewCacheFile() anyway. we may can get rid of the superfluous load()
> in makeNewCacheFile() in this case, too?
Hmmm... I don't think it is necessary
>
>>> - }
>>> + assert(path!=null);
>> I'm not friend of assert(...)....
>
> I don't care about asserts(). They don't hurt as they are not active
> anyway.
>
>> I think this is even not necessary - the self-explaining NullPointerException will be thrown little
>> bit later. However I do not insists and do as you wish.
> Feel free to remove the asserts. I automatically write them in the code,
> because they don't really hurt.
Well.. I felt free to remove them. As you told -- they should be not active O:)
>
>>> +
>>> + if (pathToURLPath(path).equals(urlPath.getPath())) { // Match found.
>>> + cacheFile = new File(path);
>>> + lruHandler.updateEntry(key);
>>> + break; // Stop searching since we got newest one already.
>>> }
>>> }
>>> return cacheFile;
>>> @@ -561,10 +532,13 @@
>>> * This will remove all old cache items.
>>> */
>>> public static void cleanCache() {
>>> +
>>> if (okToClearCache()) {
>>> // First we want to figure out which stuff we need to delete.
>>> HashSet<String> keep = new HashSet<String>();
>>> HashSet<String> remove = new HashSet<String>();
>>> +
>>> + // FIXME: lock on recently_used file needed?
>>> lruHandler.load();
>>
>> hmmm .. I do not know:(
>
> I guess it is not okay, as another javaws process could update the
> recently_used file concurrently, maybe nobody did hit this situation. I
> think the cleanChache() should be protected by a FileLock...
Ah! Now I see. This should be ok, because before cleaning always is checked whether another javaws
is not running. If is, then no cleaning. So no lock should be necessary. If I'm right (and I think
I'm then this comment should be removed)
>
>>>
>>> long maxSize = -1; // Default
>>> @@ -579,57 +553,57 @@
>>> for (Entry<String, String> e : lruHandler.getLRUSortedEntries()) {
>>> // Check if the item is contained in cacheOrder.
>>> final String key = e.getKey();
>>> - final String value = e.getValue();
>>> + final String path = e.getValue();
>>>
>>> - if (value != null) {
>>> - File file = new File(value);
>>> - PropertiesFile pf = new PropertiesFile(new File(value + ".info"));
>>> - boolean delete = Boolean.parseBoolean(pf.getProperty("delete"));
>>> + assert(path!=null);
>> same assert as above...?...
> yes. path must be not null here.
And it isn't O:)
>
>>>
...
>>> - lruHandler.removeEntry(key);
>>> +
>>> }
>>
>> I must admit that I did not got fully the meaning of this change. But it is working pretty fine and
>> looks ok :)
>
> the idea is to remove the check for "if(path != null)", as we did
> already make sure in load()->checkData() that the path is not null.
> that's all.
yap. Fair enough.
>
>>> }
>>> lruHandler.store();
>>> diff -r 093896b370d3 netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java
>>> --- a/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java Wed Mar 28 12:08:10 2012 +0200
>>> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java Wed Apr 04 16:48:09 2012 +0200
>>> @@ -713,6 +713,7 @@
>>> Runtime.getRuntime().addShutdownHook(new Thread() {
>>> public void run() {
>>> markNetxStopped();
>>> + // no lock on recently_used file needed?
>> I'm not sure again as above :(
>>> CacheUtil.cleanCache();
>>> }
>>> });
>>>
>>
>> I hope my small changes will not insult you, but while I was playing with your changeset they looks
>> to me like essential ones so I put them in instead of describing.
>
> I couldn't see any insults in this mail :-) Just good improvements to me
> patch.
>
>>
>> If you will agree with printing of lrucacheexception stacktrace then this is nearly done.
> Im' fine with the printing.
>
>> If you will insists on no stacktreace printing or (what I prefer) on printing it out just in debug
>> mode, then the tests will need little fixing. (for case with no stacktraces it will need to change
>> testing conditions, and for stacktace printed in debugmode it will need to run javaws in verbose mode)
>>
Print stacktrace moved to debug mode. In normal mode just user-friendly messages printed out. Test
modified accordingly.
If You are ok with current state, please update changelog and push. If You do not have still push
access, you can approve me to push in your name.
Best regards
J.
2012-06-04 Jiri Vanek <jvanek at redhat.com>
Thomas Meyer <thomas at m3y3r.de>
* makefile.am: (stamps/run-netx-dist-tests.stamp) and (run-reproducers-test-code-coverage.stamp)
now using $(javaws) variable instead of plaintext javaws
* netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java: (checkData) new
method checking for sanity of cache entries
(load) now checks for data sanity after loading, and stores without
corrupted items if necessary
(Comparator.compare) for sorting lru items. Now redundant checking for
sanity removed
* netx/net/sourceforge/jnlp/cache/CacheUtil.java: (getCacheFile) removed
redundant lruHandler.store call
(getCacheFileIfExist) removed iteration and cleaning mechanism
* netx/net/sourceforge/jnlp/resources/Messages.properties: modified
cache messages
* tests/jnlp_tests/signed/CacheReproducer/testcases/CacheReproducerTest.java
Added test for checking corrupted path in entry and all tests adapted for
exception thrown only in debug mode
-------------- next part --------------
A non-text attachment was scrubbed...
Name: thomasImprovedCacheFix2.diff
Type: text/x-patch
Size: 24780 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120406/8f880544/thomasImprovedCacheFix2.diff
More information about the distro-pkg-dev
mailing list