[9] RFR 8166253: (ch) FileLock object can get GC'd and result in unexpected release of file lock

[Hamlin Li]

On 01/02/2018 9:56 AM, Brian Burkhalter wrote:
> On Jan 30, 2018, at 12:58 AM, Alan Bateman <Alan.Bateman at oracle.com 
> <mailto:Alan.Bateman at oracle.com>> wrote:
>
>>> The proposed fix is:
>>>
>>> http://cr.openjdk.java.net/~bpb/8166253/webrev.03/ 
>>> <http://cr.openjdk.java.net/%7Ebpb/8166253/webrev.03/><http://cr.openjdk.java.net/%7Ebpb/8166253/webrev.03/>
>>>
>> I think it would be simpler if FileChannel maintained a set of the 
>> valid locks obtained via the channel, the the unlock can remove the 
>> lock from the set.
>
> I created and tested an updated version which maintains a set of hard 
> references to the locks for the channel inside the FileLockTable itself:
>
> http://cr.openjdk.java.net/~bpb/8166253/webrev.05/ 
> <http://cr.openjdk.java.net/%7Ebpb/8166253/webrev.05/>
>
> Given that the set is always accessed inside a block synchronized on 
> the list corresponding to the FileKey in the global 
> FileKey-to-FileLockReference table, I think the Set could probably be 
> initialized at line 124 to a simple HashSet instead of a 
> ConcurrentHashMap.KeySetView<FileLock,Boolean> but I wanted to solicit 
> comments on that point.
It looks much clear.
I'm not sure if an ConcurrentHashMap.KeySetView instance is necessary, 
as "locks" will be protected from concurrent access by "synchronized 
(list)", even if several channels might share a single list, I think it 
still works well.

In FileLockGC.java, "A hypothetical 'GC_THEN_RELEASE' case is 
infeasible", I agree. But something like below would not harm, although 
it seems unnecessary:

                  System.gc();
                  lock1.release();

>
> On Jan 30, 2018, at 9:32 PM, Hamlin Li <huaming.li at oracle.com 
> <mailto:huaming.li at oracle.com>> wrote:
>
>> From the spec, we can conclude that a java file-lock object should 
>> have the same lifetime as its channel except it's release or closed 
>> explicitly. So it's reasonable for the channel object to just have 
>> strong references to the file-lock objects, and delete references if 
>> file-lock objects are released or the channel is closed. I think in 
>> this way no new interface like FileLockListener needs to be added, 
>> and no modification is needed in FileLockImpl and FileLockTable, just 
>> needs to add/remove file-lock objects in FileChannelImpl and 
>> AsynchronousFileChannelImpl accordingly.
>
> I agree with you now. The hanging / timeout problem I observed was due 
> to the test itself and has been corrected.
good to know
>
>> And by this way, file-lock objects referred / weak-referred in 
>> FileChannelImpl.SimpleFileLockTable / 
>> FileLockTable.SharedFileLockTable have the same lifetime, so 
>> FileChannelImpl.SimpleFileLockTable can be simply removed.
>
> This is the subject of a separate issue which I have already filed and 
> hope to address this week.
OK

Thank you
-Hamlin
>
> Thanks,
>
> Brian

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/nio-dev/attachments/20180201/311d845e/attachment.html>