[rfc][icedtea-web] CacheEntry Locking Fix

[Jie Kang]

----- 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.

> 
> 
> 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.
> 
> 

Regards,

-- 

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