Skip to content

Commit f602c47

Browse files
committed
Add potential thread leak fix
1 parent 4232036 commit f602c47

File tree

3 files changed

+27
-44
lines changed

3 files changed

+27
-44
lines changed

main/src/main/java/com/sedmelluq/discord/lavaplayer/track/BaseAudioTrack.java

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,15 @@
1111
import java.util.concurrent.TimeoutException;
1212
import java.util.concurrent.atomic.AtomicBoolean;
1313
import java.util.concurrent.atomic.AtomicLong;
14+
import java.util.concurrent.atomic.AtomicReference;
1415

1516
/**
1617
* Abstract base for all audio tracks with an executor
1718
*/
1819
public abstract class BaseAudioTrack implements InternalAudioTrack {
1920
private final PrimordialAudioTrackExecutor initialExecutor;
2021
private final AtomicBoolean executorAssigned;
21-
private volatile AudioTrackExecutor activeExecutor;
22+
private final AtomicReference<AudioTrackExecutor> activeExecutor;
2223
protected final AudioTrackInfo trackInfo;
2324
protected final AtomicLong accurateDuration;
2425
private volatile Object userData;
@@ -29,7 +30,7 @@ public abstract class BaseAudioTrack implements InternalAudioTrack {
2930
public BaseAudioTrack(AudioTrackInfo trackInfo) {
3031
this.initialExecutor = new PrimordialAudioTrackExecutor(trackInfo);
3132
this.executorAssigned = new AtomicBoolean();
32-
this.activeExecutor = null;
33+
this.activeExecutor = new AtomicReference<>();
3334
this.trackInfo = trackInfo;
3435
this.accurateDuration = new AtomicLong();
3536
}
@@ -40,21 +41,25 @@ public void assignExecutor(AudioTrackExecutor executor, boolean applyPrimordialS
4041
if (applyPrimordialState) {
4142
initialExecutor.applyStateToExecutor(executor);
4243
}
43-
activeExecutor = executor;
44+
activeExecutor.set(executor);
4445
} else {
4546
throw new IllegalStateException("Cannot play the same instance of a track twice, use track.makeClone().");
4647
}
4748
}
4849

4950
@Override
5051
public AudioTrackExecutor getActiveExecutor() {
51-
AudioTrackExecutor executor = activeExecutor;
52+
AudioTrackExecutor executor = activeExecutor.get();
5253
return executor != null ? executor : initialExecutor;
5354
}
5455

5556
@Override
5657
public void stop() {
57-
getActiveExecutor().stop();
58+
AudioTrackExecutor executor = activeExecutor.getAndSet(null);
59+
if (executor == null) return;
60+
61+
initialExecutor.setPosition(executor.getPosition());
62+
executor.stop();
5863
}
5964

6065
@Override

main/src/main/java/com/sedmelluq/discord/lavaplayer/track/playback/AudioTrackExecutor.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,8 @@ public interface AudioTrackExecutor extends AudioFrameProvider {
2121
void execute(TrackStateListener listener);
2222

2323
/**
24-
* Stop playing the track, terminating the thread that is filling the frame buffer.
24+
* Stop playing the track, terminating the thread that is filling the frame buffer. Subsequent playback requires
25+
* a new executor.
2526
*/
2627
void stop();
2728

main/src/main/java/com/sedmelluq/discord/lavaplayer/track/playback/LocalAudioTrackExecutor.java

Lines changed: 15 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ public class LocalAudioTrackExecutor implements AudioTrackExecutor {
3333
private final boolean useSeekGhosting;
3434
private final AudioFrameBuffer frameBuffer;
3535
private final AtomicReference<Thread> playingThread = new AtomicReference<>();
36-
private final AtomicBoolean queuedStop = new AtomicBoolean(false);
36+
private final AtomicBoolean disposedOf = new AtomicBoolean(false);
3737
private final AtomicLong queuedSeek = new AtomicLong(-1);
3838
private final AtomicLong lastFrameTimecode = new AtomicLong(0);
3939
private final AtomicReference<AudioTrackState> state = new AtomicReference<>(AudioTrackState.INACTIVE);
@@ -56,7 +56,7 @@ public LocalAudioTrackExecutor(InternalAudioTrack audioTrack, AudioConfiguration
5656

5757
this.audioTrack = audioTrack;
5858
AudioDataFormat currentFormat = configuration.getOutputFormat();
59-
this.frameBuffer = configuration.getFrameBufferFactory().create(bufferDuration, currentFormat, queuedStop);
59+
this.frameBuffer = configuration.getFrameBufferFactory().create(bufferDuration, currentFormat, disposedOf);
6060
this.processingContext = new AudioProcessingContext(configuration, frameBuffer, playerOptions, currentFormat);
6161
this.useSeekGhosting = useSeekGhosting;
6262
}
@@ -92,7 +92,15 @@ public void execute(TrackStateListener listener) {
9292
log.debug("Cleared a stray interrupt.");
9393
}
9494

95-
if (playingThread.compareAndSet(null, Thread.currentThread())) {
95+
synchronized (actionSynchronizer) {
96+
if (disposedOf.get()) {
97+
log.warn("Attempt to execute executor that has been disposed of");
98+
return;
99+
}
100+
}
101+
102+
boolean wasUpdated = playingThread.compareAndSet(null, Thread.currentThread());
103+
if (wasUpdated) {
96104
log.debug("Starting to play track {} locally with listener {}", audioTrack.getInfo().identifier, listener);
97105

98106
state.set(AudioTrackState.LOADING);
@@ -105,7 +113,7 @@ public void execute(TrackStateListener listener) {
105113
// Temporarily clear the interrupted status so it would not disrupt listener methods.
106114
interrupt = findInterrupt(e);
107115

108-
if (interrupt != null && checkStopped()) {
116+
if (interrupt != null) {
109117
log.debug("Track {} was interrupted outside of execution loop.", audioTrack.getIdentifier());
110118
} else {
111119
frameBuffer.setTerminateOnEmpty();
@@ -140,31 +148,18 @@ public void execute(TrackStateListener listener) {
140148
@Override
141149
public void stop() {
142150
synchronized (actionSynchronizer) {
151+
disposedOf.set(true);
143152
Thread thread = playingThread.get();
144153

145154
if (thread != null) {
146155
log.debug("Requesting stop for track {}", audioTrack.getIdentifier());
147-
148-
queuedStop.compareAndSet(false, true);
149156
thread.interrupt();
150157
} else {
151158
log.debug("Tried to stop track {} which is not playing.", audioTrack.getIdentifier());
152159
}
153160
}
154161
}
155162

156-
/**
157-
* @return True if the track has been scheduled to stop and then clears the scheduled stop bit.
158-
*/
159-
public boolean checkStopped() {
160-
if (queuedStop.compareAndSet(true, false)) {
161-
state.set(AudioTrackState.STOPPING);
162-
return true;
163-
}
164-
165-
return false;
166-
}
167-
168163
/**
169164
* Wait until all the frames from the frame buffer have been consumed. Keeps the buffering thread alive to keep it
170165
* interruptible for seeking until buffer is empty.
@@ -176,24 +171,6 @@ public void waitOnEnd() throws InterruptedException {
176171
frameBuffer.waitForTermination();
177172
}
178173

179-
/**
180-
* Interrupt the buffering thread, either stop or seek should have been set beforehand.
181-
*
182-
* @return True if there was a thread to interrupt.
183-
*/
184-
public boolean interrupt() {
185-
synchronized (actionSynchronizer) {
186-
Thread thread = playingThread.get();
187-
188-
if (thread != null) {
189-
thread.interrupt();
190-
return true;
191-
}
192-
193-
return false;
194-
}
195-
}
196-
197174
@Override
198175
public long getPosition() {
199176
long seek = queuedSeek.get();
@@ -336,7 +313,7 @@ private void interruptForSeek() {
336313
private boolean handlePlaybackInterrupt(InterruptedException interruption, SeekExecutor seekExecutor) {
337314
Thread.interrupted();
338315

339-
if (checkStopped()) {
316+
if (disposedOf.get()) {
340317
markerTracker.trigger(STOPPED);
341318
return false;
342319
}
@@ -345,7 +322,7 @@ private boolean handlePlaybackInterrupt(InterruptedException interruption, SeekE
345322

346323
if (seekResult != SeekResult.NO_SEEK) {
347324
// Double-check, might have received a stop request while seeking
348-
if (checkStopped()) {
325+
if (disposedOf.get()) {
349326
markerTracker.trigger(STOPPED);
350327
return false;
351328
} else {

0 commit comments

Comments
 (0)