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

[Sergey Bylokhov]

On Mon, 24 May 2021 12:16:15 GMT, Alexander Zvegintsev <azvegint 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`.

It could make sense only if the "sendEvents" flag will be used under some kind of synchronization as well, otherwise, it is possible to get sendEvents=true twice, it was not changed after this fix:
synchronized (this) {
    if (this.started != started) {
        this.started = started;
        sendEvents = true;
      }
}
<<< here the other thread could set "started" to the opposite state and we post events twice.
if (sendEvents) {
.....
}

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

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