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

[Daniel Fuchs]

Hi Brian,

NIO is not my area of expertise, but I have some questions WRT
to MT-safety of the proposed changes:

FileChannelImpl.java:

   - is it safe for fileLockState to be a simple ArrayList?
     It seems to be modified outside of any critical section
     (lock/tryLock), and it is also iterated over from outside
     any critical section (line 137), so should this be a
     CopyOnWriteArrayList instead?

FileLockImpl.java:

   - is there any reason while state is not final here?
     if it were final then maybe 'invalid' would no longer
     be needed - as it seems to be a duplicate of state.invalid

FileLockTable.java:

   - The 'states' list is a simple ArrayList, which means
     that removeKeyIfStatesEmpty(fileKey, states) should
     be called while holding a lock on 'states', to make
     sure that nothing gets added to states while the
     method is running.
     This is good for line 295, but I believe line 348
     should be moved up just after line 346 in order
     to be within the critical section - shouldn't it?

best regards,

-- daniel

On 05/11/16 19:14, Brian Burkhalter wrote:
> This review request supersedes the previous .00 version [1] for this issue. The current .01 version of the patch as proposed here is incomplete as it does not address AsynchronousFileChannels. I wanted to post it at this time however as it seems overcomplicated and I do not wish to expend more effort on it unless it is actually headed in the right direction.
>
> Issue:	https://bugs.openjdk.java.net/browse/JDK-8166253
> Patch:	http://cr.openjdk.java.net/~bpb/8166253/webrev.01/
>
> The FileLocks for a given file are maintained via an internal table wherein the values are lists of WeakReferences to the FileLock instances corresponding to a given channel (file). No hard references to the FileLocks are maintained as this could prevent garbage collection of the FileChannel associated with the FileLock as the latter contains a hard reference to the former. The problem is that if the calling code which requests the lock does not maintain a hard reference to the FileLock, then it could be GCed without actually releasing the underlying native lock. In this situation it appears to the Java layer that the lock no longer exists although it has not been released by the native layer. This could result in unexpected behavior both within the Java process itself and between it and other processes using locks on the file.
>
> The approach here is to keep track of the state of valid FileLocks which have become unreachable and have been removed from the associated ReferenceQueue. The FileLockState maintains the position, size, and validity of the lock. The shared status of the lock is irrelevant as the state if used is only to release the lock which does not require specifying shared or exclusive. A reference to the FileLockState is maintained in the FileLock instance, by the FileChannel on which the lock was obtained, and in the FileLockReference. The FileLockState is invalidated if the FileLock is invalidated or if the channel is closed, whichever occurs first.
>
> In FileLockTable.removeStaleEntries(), if it is determined that the FileLockState of the lock removed from the ReferenceQueue is still valid, the FileLockState is added to a global table under the same key as that used for the lock in the global table of lists of FileLockReferences. If the FileLockState is invalid, then the state instance is removed from that table.
>
> In FileLockTable.checkList(), the list of FileLockStates is checked for the file key to see whether any valid orphaned states overlap the requested region and if so an exception is thrown. Any invalid states encountered are removed from the list and if the list becomes empty it and it’s key are removed from the table. The handling of the contents of the table of states was mirrored from that of the table of lists of FileLockReferences.
>
> In FileChannelImpl, a list of FileLockStates of locks acquired on the channel is maintained. In implCloseChannel() this list is traversed and any regions associated with valid states are released and the state instance invalidated.
>
> This patch passes the core NIO regression tests on all the usual platforms. The test included in the patch fails on all platforms if the implementation portion of the patch is omitted.
>
> Thanks,
>
> Brian
>
> [1] http://mail.openjdk.java.net/pipermail/nio-dev/2016-September/003881.html
>