[rfc][icedtea-web] CacheEntry Locking Fix

[Jie Kang]

----- Original Message -----
> 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,
> 

Hello,

Thanks for the review :D


> Two nits
>   - you should always unlock in finally clause.

I've searched through the code and moved all applicable unlocks into 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.

I've added two tests, one for testing Multiple Writes While Locked and one for Reading While Locked.


Regards,

> 
> 
> J.
> 
> Please add those two tests.
> 
> 
> 
> 

-- 

Jie Kang
-------------- next part --------------
A non-text attachment was scrubbed...
Name: itw-cache-lock-2.patch
Type: text/x-patch
Size: 24074 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140819/201307d0/itw-cache-lock-2-0001.patch>