[rfc][icedtea-web] CacheEntry Locking Fix

[Jie Kang]

----- 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: itw-lock-removal.patch
Type: text/x-patch
Size: 8143 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140806/e128ebae/itw-lock-removal-0001.patch>