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

[Brian Burkhalter]

Hi Daniel,

Thank you for your useful comments.

On Nov 7, 2016, at 8:10 AM, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:

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

I am not surprised that there are problems of this nature. I made some of the modifications to my prototype quickly with the objective of publishing the review request sooner as I am not certain whether this is conceptually even the best way to address the issue.

> 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?

Indeed that looks to be a better choice.

> 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

Good point. Probably it would be better always to initialize the state in the FileLockImpl constructor and remove the latter’s ‘invalid’ instance variable.

> 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?

You are correct.

I’ll make the suggested changes.

Thanks,

Brian

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