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