[rfc][icedtea-web] CacheEntry Locking Fix

[Jiri Vanek]

On 08/18/2014 07:41 PM, Jie Kang wrote:
>
>
> ----- Original Message -----
>> On 08/06/2014 07:24 PM, Jie Kang wrote:
>>>
>>>
>>> ----- Original Message -----
>>>> On 08/06/2014 03:20 PM, Jie Kang wrote:
>>>>>
>>>>>
>>>>> ----- Original Message -----
>>>>>> On 08/06/2014 03:00 PM, Jie Kang wrote:
>>>>>>>
>>>>>>>
>>>>>>> ----- Original Message -----
>>>>>>>> On 08/05/2014 09:08 PM, Jie Kang wrote:
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>> I discovered that the CacheEntry locking system currently does not
>>>>>>>>> work
>>>>>>>>> as
>>>>>>>>> expected. Calls to lock() or unlock() have no real purpose as they do
>>>>>>>>> not
>>>>>>>>> prevent the properties file from being read or written to. The patch
>>>>>>>>> addresses this by preventing writes from the CacheEntry unless one
>>>>>>>>> has
>>>>>>>>> acquired a lock on the properties file.
>>>>>>>>>
>>>>>>>>> There are also two new tests that demonstrate the issue.
>>>>>>>>>
>>>>>>>>> The first test 'verifyLockCannotBeUnlocked' tests whether or not
>>>>>>>>> CacheEntries can unlock each other's lock. Prior to this patch, if
>>>>>>>>> CacheEntry A 'lock's the properties file, CacheEntry B could call
>>>>>>>>> 'unlock'
>>>>>>>>> and release the lock for entry A. This is no longer possible.
>>>>>>>>>
>>>>>>>>> The second test 'verifyLockPreventsWrite' tests whether or not
>>>>>>>>> another
>>>>>>>>> CacheEntry can write to the properties file while some other entry
>>>>>>>>> has
>>>>>>>>> the
>>>>>>>>> lock. Prior to this patch, if CacheEntry A 'lock's the properties
>>>>>>>>> file,
>>>>>>>>> CacheEntry B could still read and write from/to the same file. This
>>>>>>>>> is
>>>>>>>>> no
>>>>>>>>> longer possible.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> One issue this does not address is that the :: public void function
>>>>>>>>> store()
>>>>>>>>> :: in PropertiesFile.java can still be called without a lock. This
>>>>>>>>> has
>>>>>>>>> not
>>>>>>>>> been refactored as of yet due to CacheLRUWrapper.java's use of
>>>>>>>>> store()
>>>>>>>>> without the need of locking (I think). So it is still up to the
>>>>>>>>> developer
>>>>>>>>> to make sure he/she writes code that makes use of the locking system
>>>>>>>>> when
>>>>>>>>> dealing with PropertiesFile.java directly. The patch does make sure
>>>>>>>>> developers cannot screw up when dealing with CacheEntry.java.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> uff... Does this issue have some real consequences?
>>>>>>>
>>>>>>>
>>>>>>>     From what I have seen, no.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I am not sure what the original developers had in mind. At the moment,
>>>>>>> it
>>>>>>> functions as an advisory lock[1] which has questionable usefulness. The
>>>>>>> lock is placed on the .info files that we create and place in cache.
>>>>>>> You
>>>>>>> could remove the whole lock system and in the huge majority of cases,
>>>>>>> see
>>>>>>> no changes in behaviour. (what other programs are going to lock our
>>>>>>> .info
>>>>>>> files?)
>>>>>>>
>>>>>>> My fix assumes that we actually wanted the locks to work within the JVM
>>>>>>> and
>>>>>>> makes it do so.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> [1]
>>>>>>> http://docs.oracle.com/javase/7/docs/api/java/nio/channels/FileLock.html
>>>>>>> Maybe someone has more experience with FileLock but from what I read it
>>>>>>> is
>>>>>>> just a lock held on behalf of JVM (so not useful for multiple threads
>>>>>>> in
>>>>>>> one JVM) and is also system dependent so it should be assumed to be an
>>>>>>> advisory lock where other programs need to observe the same locking
>>>>>>> system
>>>>>>> (if they don't, they can still read/write to the file we lock).
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>> I guess the original locking was designed that multipl einstances of
>>>>>> netx
>>>>>> do
>>>>>> not owerwrite cache
>>>>>> under each hands. Butit seems that it do not happen.
>>>>>
>>>>> I was of understanding that the instances of netx run in the same JVM?
>>>>>
>>>>>>
>>>>>>
>>>>>> The netx is already overlocked, and we got reports of dropping
>>>>>> performance
>>>>>> if
>>>>>> to much instances are
>>>>>> running.
>>>>>>
>>>>>>
>>>>>> As this really do not seem to have impact on live, I would rather not
>>>>>> added
>>>>>> another locks into
>>>>>> icedtea-web.
>>>>>>
>>>>>> I'm terribly sorry for dishonoring this effort - as it is an nice coe
>>>>>> and
>>>>>> it
>>>>>> *do8 fix an bug. But I
>>>>>> really have  strong concerns.
>>>>>>
>>>>>>
>>>>>> I'm more in temptation to remove this dead code or maybe add known to
>>>>>> fail
>>>>>> (no no no!) unittests?
>>>>>
>>>>> Yes there is option to just remove dead code. Also the two unit tests I
>>>>> added could be marked KoF.
>>>>>
>>>>>
>>>>> I think of the two, I prefer removal. But should probably get other
>>>>> opinions too. The locking system is extremely simple so can easily be
>>>>> added back later (if ever).
>>>>
>>>> Ok. Please try to remove dead code. lets see how it loosks like. For two
>>>> tests - yes please -add
>>>> them  and mark @ignore  and comment.
>>>
>>> Hello,
>>>
>>>
>>> Patch attached.
>>>
>>> I wasn't entirely what you meant by comment, I have added a description for
>>> the two tests as to why the @Ignore is there and also commented out the
>>> code since it no longer compiles (since dead code is removed). Is this
>>> okay?
>>
>> Hi.
>>
>> Well - sorry for sending you there, here, there and here again. But I think
>> that mayby this locking
>> mechanism should be ncluded at the end.
>
>
> Hello,
>
> No worries! The patch is pretty small and only took a short time to write :D
>
> I have attached a reworked version of the patch.
>
>>
>>
>> But Iwould suggest some of already existing features which already are in ITW
>> - see
>> net.sourceforge.jnlp.util.lockingfile.*
>
> I have implemented the patch using classes in this package. Thanks for the heads up, I didn't see this package before x-x;
>
>
> Of course with locking we must be careful. I will continue to test this and ask ldracz for help to make sure I didn't break anything.
>

Agree..

I'm moreover happy with this patch,

Two nits
  - you should always unlock in finally clause.
  - the unittests - the tests you had in previouspatch (the ignored two) are not applicable?
   - if so,please, add
   - the unittest testing the locking feature is actually missing


something like start thread one, lock and write write write. Start thread two and try to read 
(should be ok) start thread three and try to write - shouldnot write a single bite. Pelase add those 
teo tests.


J.

Please add those two tests.