[rfc][icedtea-web] CacheEntry Locking Fix

[Jiri Vanek]

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.


But Iwould suggest some of already existing features which already are in ITW - see 
net.sourceforge.jnlp.util.lockingfile.*


I think this lock should be used for this patch.

If the effort will be lesser then in orriginal patch then maybe.... Sorry for  hesitating on this 
issue :(


J.