Skip to content

8266421: Deadlock in Sound System #4141

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

8266421: Deadlock in Sound System #4141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a852f51
Create SetPositionHang.java
mrserb May 21, 2021
9f36487
Update SetPositionHang.java
mrserb May 21, 2021
aedecbf
Initial fix
mrserb May 21, 2021
2025e14
Update SetPositionHang.java
mrserb Jun 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 5 additions & 14 deletions src/java.desktop/share/classes/com/sun/media/sound/AbstractDataLine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1999, 2019, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1999, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -295,13 +295,7 @@ final void setActive(boolean active) {
//boolean sendEvents = false;
//long position = getLongFramePosition();

synchronized (this) {

if (this.active != active) {
this.active = active;
//sendEvents = true;
}
}
this.active = active;

// $$kk: 11.19.99: take ACTIVE / INACTIVE / EOM events out;
// putting them in is technically an API change.
Expand All @@ -324,12 +318,9 @@ final void setStarted(boolean started) {
boolean sendEvents = false;
long position = getLongFramePosition();

synchronized (this) {

if (this.started != started) {
this.started = started;
sendEvents = true;
}
if (this.started != started) {
this.started = started;
Comment on lines +321 to +322
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
.....
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The case with setting opposite state you are describing is a different one. We will post two different events like start and stop.

I am talking about calling setStarted() with same parameter from different threads.

For example we have two threads T1 and T2 calling setStarted(true).
Before the fix only one of thread will set sendEvents to true due to synchronized block.

After the fix following is possible:

// this.started  == false, started == true
if (this.started != started) { //T1 passed this check
    // <<<<< at this moment T2 checks the statement above
    this.started = started; // T1 and T2 assigns same new value to this.started
    sendEvents = true; // both threads sets sendEvents to true.
}
// sending two start events.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My example was about the thread-safety of the method, after synchronized block, the other thread may change the state, then post event, and then this thread will post event. So we will post the event twice in the wrong order. So this method as-is is not thread-safe, we should not call it in parallel. I'll post data for its usage here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The changes in the AbstractDataLine.setActive() are fine, we do not post events there.
  2. The changes in the AbstractDataLine.setStarted() are used and syncronised:
    used in AbstractDataLine:343- the setEOM() dead unused non-public method
    used in AbstractDataLine:205 - syncronised on synchronized(mixer); Also dead code, the setStarted() is never executed, since the line is stopped already a few lines above in the implStop();
    used in DirectAudioDevice:533 - synchronized on synchronized(lock) and synchronized(mixer)
    used in DirectAudioDevice:560 - synchronized on synchronized(lock) and synchronized(mixer)
    used in DirectAudioDevice:707 - synchronized on synchronized(lock)
    used in DirectAudioDevice:932 - synchronized on synchronized(lock)
  3. The changes in the AbstractLine.setOpen() are used and synchronised:
    used in AbstractDataLine:118 - syncronised on synchronized(mixer);
    used in AbstractDataLine:374 - syncronised on synchronized(mixer);
    used in AbstractMixer:288 - syncronised on this;
    used in AbstractMixer:377 - syncronised on this;
    used in PortMixer:277 - syncronised on synchronized(mixer);
    used in PortMixer:293 - syncronised on synchronized(mixer);

So every object uses its own high-level synchronization when it calls the methods in question.

sendEvents = true;
}

if (sendEvents) {
Expand Down
10 changes: 4 additions & 6 deletions src/java.desktop/share/classes/com/sun/media/sound/AbstractLine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1999, 2019, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1999, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -159,11 +159,9 @@ final void setOpen(boolean open) {
boolean sendEvents = false;
long position = getLongFramePosition();

synchronized (this) {
if (this.open != open) {
this.open = open;
sendEvents = true;
}
if (this.open != open) {
this.open = open;
sendEvents = true;
}

if (sendEvents) {
Expand Down
4 changes: 2 additions & 2 deletions src/java.desktop/share/classes/com/sun/media/sound/DirectAudioDevice.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2002, 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2002, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -1192,7 +1192,7 @@ public long getLongFramePosition() {
}

@Override
public synchronized void setMicrosecondPosition(long microseconds) {
public void setMicrosecondPosition(long microseconds) {
long frames = Toolkit.micros2frames(getFormat(), microseconds);
setFramePosition((int) frames);
}
Expand Down
106 changes: 106 additions & 0 deletions test/jdk/javax/sound/sampled/Clip/SetPositionHang.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
/*
* Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

import javax.sound.sampled.AudioFormat;
import javax.sound.sampled.AudioSystem;
import javax.sound.sampled.Clip;
import javax.sound.sampled.LineUnavailableException;

/**
* @test
* @bug 8266421
* @summary Tests that Clip.setFramePosition/setMicrosecondPosition do not hang.
*/
public final class SetPositionHang implements Runnable {

private static volatile boolean testFramePosition;
private final Clip clip;
private final String thread;

private SetPositionHang(String thread, Clip clip) {
this.thread = thread;
this.clip = clip;
}

public static void main(String[] args) throws Exception {
testFramePosition = false;
test();
testFramePosition = true;
test();
}

private static void test() throws InterruptedException {
try (Clip clip = AudioSystem.getClip()) {
// prepare audio data
int frameCount = 441000; // lets say 10 seconds
AudioFormat format = new AudioFormat(44100.0f, 16, 2, true, false);
byte[] bytes = new byte[frameCount * format.getFrameSize()];

clip.open(format, bytes, 0, frameCount);
Thread t1 = new Thread(new SetPositionHang("1", clip));
Thread t2 = new Thread(new SetPositionHang("2", clip));
Thread t3 = new Thread(new SetPositionHang("3", clip));
Thread t4 = new Thread(new SetPositionHang("4", clip));
Thread t5 = new Thread(new SetPositionHang("5", clip));
t1.start();
t2.start();
t3.start();
t4.start();
t5.start();
t1.join();
t2.join();
t3.join();
t4.join();
t5.join();
} catch (LineUnavailableException | IllegalArgumentException ignored) {
// the test is not applicable
}
}

public void run() {
System.out.println("Thread " + thread + " Start");
for (int i = 0; i < 100; i++) {
// System.out.println("Thread " + thread + " Play " +
// System.currentTimeMillis() % 100000);
playSound();
try {
Thread.sleep(i);
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
}
System.out.println("Thread " + thread + " Finish");
}

private void playSound() {
if (clip.isRunning()) {
clip.stop();
}
if (testFramePosition) {
clip.setFramePosition(0);
} else {
clip.setMicrosecondPosition(0);
}
clip.start();
}
}