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

[Hamlin Li]

On 31/01/2018 11:04 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 don’t see how to do this. The FileLock already holds a reference to 
> the Channel and if the converse were to obtain there would be circular 
> references and things blocked from being collected. 
Hi Brian,

I don't have expertise in this area, but I'd like to share my points.

First, I think JVM GC can deal with circular references, so it's not an 
issue.

Then Let me try to explain what Alan means:
As you have pointed out the content in FileLock spec:
"A file-lock object is initially valid. It remains valid until the lock 
is released by invoking the release method, by closing the channel that 
was used to acquire it, or by the termination of the Java virtual 
machine, whichever comes first."
 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. 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.

> I did however come up with a simpler change which passes the new test:
>
> http://cr.openjdk.java.net/~bpb/8166253/webrev.04/ 
> <http://cr.openjdk.java.net/%7Ebpb/8166253/webrev.04/>
Second, although it will solve JDK-8166253, it will introduce a new 
issue by line 290("if (!ref.isValid()) {") in FileLockTable.java. I 
think all the refs in queue will satisfy the condition "ref.isValid()", 
so there will be no ref be removed by removeStaleEntries, it will 
increase queue infinitely, finally could get out of memory if long 
enough. And seems there is workaround for this new issue, because line 
290 is the key point to fix the JDK-8166253.

So I prefer Alan's suggestion, it's more clear and straight.

Thank you
-Hamlin
>
> More comprehensive testing would need to be done if this looks better.
>
> On Jan 30, 2018, at 2:16 AM, Daniel Fuchs <daniel.fuchs at oracle.com 
> <mailto:daniel.fuchs at oracle.com>> wrote:
>
>> Should fileKey be made final (AFAICS the field
>> is never mutated) and should isLockReleased
>> be volatile (appears that it could be mutated/read
>> from different threads)?
>
> Fixed in the above webrev.
>
> Thanks,
>
> Brian

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