On Fri, 21 May 2021 08:15:25 GMT, Sergey Bylokhov <s...@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
