[rfc][icedtea-web] CacheEntry Locking Fix

[Jiri Vanek]

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.

ty!