[rfc][icedtea-web] CacheEntry Locking Fix

[Jie Kang]

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

> 
> 
> J.
> 
> 

-- 

Jie Kang