<Sound Dev> RFR: 8266421: Deadlock in Sound System

[Alexander Zvegintsev]

On Fri, 21 May 2021 08:15:25 GMT, Sergey Bylokhov <serb at openjdk.org> wrote:

> In the fix for JDK-8207150 I have updated the synchronization of some code paths under one "lock", before that code was synchronized only on some threads and missing on others. Old review:
> http://mail.openjdk.java.net/pipermail/sound-dev/2018-August/000650.html
> 
> That fix introduced this order of locks: "lock"->"synchronized(this)", I have checked other places and did not found where we use the opposite order. Unfortunately, one such place exists in the private subclass DirectClip.
> 
> I have checked both usages of synchronized which caused deadlock:
>  - In the DirectClip class the method setMicrosecondPosition is marked as "synchronized" but it is unclear why. This method is implemented as a call to another public method setFramePosition which is not "synchronized" and use some internal synchronization. So I have removed this keyword.
>  - In a few places we have the code like this:
> 
>         boolean sendEvents = false;
>         synchronized (this) {
>             if (this.started != started) {
>                 this.started = started;
>                 sendEvents = true;
>             }
> 	}
>         if (sendEvents) {.....
> 
> I doubt that this type of synchronization may help something - the fields are volatile and we use sendEvents flag after synchronisation block, so I removed it as well. Any thoughts?

src/java.desktop/share/classes/com/sun/media/sound/AbstractDataLine.java line 322:

> 320: 
> 321:         if (this.started != started) {
> 322:             this.started = started;

> I doubt that this type of synchronization may help something - the fields are volatile and we use sendEvents flag after synchronization block, so I removed it as well. Any thoughts?

These fields are volatile, but the comparison and assignment is not atomic.
So I believe it is possible the case when we will have `sendEvents == true` in two threads, hence we will send a duplicate event.

So we probably might want to use `AtomicBoolean` if you want to get rid of `synchronized`.

test/jdk/javax/sound/sampled/Clip/SetPositionHang.java line 84:

> 82:         for (int i = 0; i < 100; i++) {
> 83:             System.out.println("Thread " + thread + " Play "
> 84:                                        + System.currentTimeMillis() % 100000);

This `println` spams a lot.
It took a ~1 second less to complete the test when these lines are commented(~8s vs ~9s, I believe it will took more on slower machines).
So we probably could comment out these lines to reduce test execution time.

-------------

PR: https://git.openjdk.java.net/jdk/pull/4141