<Sound Dev> [10] Review Request: 8178403 DirectAudio in JavaSound may hang and leak

[Phil Race]

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