JDK-8054039 Deadlock during interrupting FileChannel

[Frank Yuan]

Hi Brian

 

Thank you for review and your comments:)

 

The size of my last mail is too large, is still held by maillist moderator, I have to send some content again(the diagram still
can't be attached.).

 

The bug is a dead lock may happen during a FileChannel is being interrupted. 

 

With applying attached <<deadlock.diff>>, in which I add some delay code at some points, the dead lock happens very easy.

 

I have 2 solutions to fix this issue. The first one is to be like the attached fix1.diff, if I understand correctly, in
signalAndWait(), Thread.currentThread().interrupt() is just to set interrupted flag again, the whole interrupt() is unnecessary, in
my <<fix1.diff>>, I invoke Thread. interrupt0() by reflection, this is not the final code, just an example.

 

Brian's comment:

I am suspicious of the use of reflection here.

I also have some concern for this solution, because in this fix nio will depend on the private method of Thread -- interrupt0, it
will break the encapsulation. However it's an available solution in theory.

 

 

The second solution has been attached in https://bugs.openjdk.java.net/secure/attachment/28435/8054039.diff. In this fix, I replace
closeLock in AbstractInterruptibleChannel.java with AtomicBoolean and compareAndSet(), that eliminates one of the locks, like my
solution 1.

 

Brian's comment:

Within the scope of AbstractInterruptibleChannel I do not see any problem with the change that you propose. Given that the affected
instance variables are private to the class and that AtomicBoolean "generally" follows the rules for volatiles it looks conceptually
valid. I do not know however about the implications with respect to interactions with other classes. For example, could this change
provoke some unforeseen behavior as a result of subtle changes in timing?

 

As my understood, this solution won't spend more time on begin() than the old code, compareAndSet is atomic and there is no loop or
wait. Anyway I am not sure your concern, will do more tests, hope more people can review it.

 

 

Best Regards

Frank

 

 

From: Brian Burkhalter [mailto:brian.burkhalter at oracle.com] 
Sent: Wednesday, September 09, 2015 7:26 AM
To: Frank Yuan <frank.yuan at oracle.com>; nio-dev <nio-dev at openjdk.java.net>
Cc: FELIX YANG <felix.yang at oracle.com>
Subject: Re: JDK-8054039 Deadlock during interrupting FileChannel

 

Hi Frank,

 

Sorry that this topic has languished for so long with no comment.

 

On Sep 8, 2015, at 2:47 AM, Frank Yuan <frank.yuan at oracle.com <mailto:frank.yuan at oracle.com> > wrote:





Thread 1 and thread 2 invoke map() on a same FileChannel, at same time, thread 3 is interrupting thread 2, if it happens to meet the
conditions at the 4 time points, thread 2 will hold closeLock of the FileChannel and try to get the blockerLock of itself(i.e.
thread 2), thread 3 will hold blockerLock of thread 2 and try to get closeLock of the FileChannel, that will result in the dead
lock.

 

This analysis appears to me to be accurate.





 With applying attached deadlock.diff, in which I add some delay code at some points, the dead lock happens very easy, refer to the
attached <<test InterruptMapDeadlock fail.txt>>, if you use agentvm mode, jtreg will even report the dead lock found, see <<jtreg
report dead lock.txt >>

 

I have 2 solutions to fix this issue. The first one is to be like the attached fix1.diff, if I understand correctly, in
signalAndWait(), Thread.currentThread().interrupt() is just to set interrupted flag again, the whole interrupt() is unnecessary, in
my fix1.diff, I invoke Thread. interrupt0() by reflection, this is not the final code, just an example.

 

I am suspicious of the use of reflection here.





The second solution is << 8054039.diff >>, which has been attached in
<https://bugs.openjdk.java.net/secure/attachment/28435/8054039.diff>
https://bugs.openjdk.java.net/secure/attachment/28435/8054039.diff. In this fix, I replace closeLock in
AbstractInterruptibleChannel.java with AtomicBoolean and compareAndSet(), that eliminates one of the locks, like my solution 1.

 

Within the scope of AbstractInterruptibleChannel I do not see any problem with the change that you propose. Given that the affected
instance variables are private to the class and that AtomicBoolean "generally" follows the rules for volatiles it looks conceptually
valid. I do not know however about the implications with respect to interactions with other classes. For example, could this change
provoke some unforeseen behavior as a result of subtle changes in timing?

 

As for picky comments, the source file could use some minor cleanup in the form of removing unused imports and adding @Override
annotations where appropriate.

 

Thanks,

 

Brian

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/nio-dev/attachments/20150909/e9ce9112/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix1.diff
Type: application/octet-stream
Size: 832 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/nio-dev/attachments/20150909/e9ce9112/fix1-0001.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: deadlock.diff
Type: application/octet-stream
Size: 2292 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/nio-dev/attachments/20150909/e9ce9112/deadlock-0001.diff>