[rfc][icedtea-web] CacheEntry Locking Fix

[Jie Kang]

----- Original Message -----
> 
> 
> ----- 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?
> 
> > 
> > ty!
> > 
> 
> 
> Regards,
> 
> 
> --
> 
> Jie Kang
> 

Ping.


Regards,

-- 

Jie Kang