This is a tricky piece of code with loops, thread comparison checks,
a volatile variable, synchronized blocks with monitors and I don't
find it easy to review without a better understanding of all of
how all of these parts are used here.
thread is assigned on creation and we are checking it in run() so
(thread == curThread) seems like it must be true until somehow
implClose() is called :
1300 Thread oldThread = thread;
1301 thread = null;
1302 doIO = false;
1303 if (oldThread != null) {
So why not just check if (thread == null) instead ?
And when can oldThread will be null ? If someone called stop twice ?
And why is there no thread = null assignment in implClose() ?
Shouldn't you update the test to add this new bug id ?
-phil.
On 07/05/2017 07:56 AM, Sergey Bylokhov wrote:
Hello,
Please review the fix for jdk10.
Bug: https://bugs.openjdk.java.net/browse/JDK-8178403
Webrev can be found at: http://cr.openjdk.java.net/~serb/8178403/webrev.00
This fix is updated version of JDK-8168751[1].
The problem is in this lines of code
1349 while (thread == curThread) {
1350 // doIO is volatile, but we could check it, then get
1351 // pre-empted while another thread changes doIO and
notifies,
1352 // before we wait (so we sleep in wait forever).
1353 synchronized(lock) {
1354 if (!doIO) {
1355 try {
1356 lock.wait();
"thread == curThread" is executed outside the synchronized block, so it is possible that
this condition will be changed after if() but before "lock.wait();", so the code will
waits forever because nobody will call lock.notifyAll().
Also the fix calls the "lock.wait();" in the loop, so we will have two
symmetric loops one to wait() and one to play().
The test is updated to reproduce the bug more often.
[1] https://bugs.openjdk.java.net/browse/JDK-8168751
