Correct control of execution of Java thread

Question

I'm writing an application that will use the JLayer library to play an mp3 file. Since the player does not have any methods to pause the playback - play() does not return until the song is finished - I decided to put it in a thread and simply control that thread. This is possible because one can execute play(1) which plays a single frame of the song. Putting this in a loop will thus play the entire song smoothly.

What I have done is the following:

Create a Thread class Mp3PlayerThread that can be used to play a song in a different thread.
Create a wrapping class Mp3Player that holds a thread object and controls the volatile flags inside the Thread.

I built the object Mp3Player such that it should be thread safe. I.e., pass an instance of Mp3Player around. To do this I made all the control methods synchronized.

I am wondering if this code can be made more solid without using volatile variables? I have always learned that you should not use the volatile keyword if possible.

Mp3PlayerThread

public class Mp3PlayerThread extends Thread
{
    public  volatile boolean playing = false;
    public  volatile boolean abort   = false;
    private Player   player;
    private String   path;

    public Mp3PlayerThread(String file)
    {
        this.path = file;
    }

    public void run()
    {
        // http://stackoverflow.com/questions/7313657/should-you-synchronize-the-run-method-why-or-why-not
        try
        {
            FileInputStream     fis = new FileInputStream(this.path);
            BufferedInputStream bis = new BufferedInputStream(fis);
            this.player             = new Player(bis);

            while (!abort)
            {
                if (playing)
                    player.play(1);
            }
            Printer.debugMessage(this.getClass(), "end of file or stopped");

        } catch (FileNotFoundException | JavaLayerException e)
        {
            e.printStackTrace();
        }
    }
}

Mp3Player

public class Mp3Player
{
    private Mp3PlayerThread playerThread;
    private String          song;

    public Mp3Player(String filename)
    {
        song = filename;
    }
    /**
     * Initializes the playerThread with the given song, sets the flag to true
     * and starts playing the song.
     */
    public synchronized void play()
    {
        playerThread         = new Mp3PlayerThread(song);
        playerThread.playing = true;
        playerThread.start();
    }
    /**
     * Pauses the execution of the playerThread.
     */
    public synchronized void pause()
    {
        Printer.debugMessage(this.getClass(), "paused");
        playerThread.playing = false;
    }
    public synchronized void resume()
    {
        Printer.debugMessage(this.getClass(), "resuming");
        playerThread.playing = true;
    }
    public synchronized void stop()
    {
        Printer.debugMessage(this.getClass(), "stopping");
        playerThread.abort = true;
        playerThread       = null; // Needed or not?
    }
}

unholysampler · Accepted Answer · 2014-08-28 17:59:50Z

Lets start with the instance variables.

public class Mp3PlayerThread extends Thread
{
    public  volatile boolean playing = false;
    public  volatile boolean abort   = false;
    private Player   player;
    private String   path;
    //...
}

You shouldn't be directly exposing playing and abort. Doing this hinders your ability to change the internal implementation of your class and remain backwards compatible.
Trying to line up you variable declarations will be a losing battle. It looks nice now, but when you add a new variable, you might have to completely change everything. This will make looking at diffs harder because some lines will be changed even though they are not directly involved in the new functionality. Additionally, an argument could be made that all of the variable names should be lined up instead of having player line up with boolean.
path is set once in the constructor and never changed. Setting it to be final could prevent you from accidentally changing it later with the expectation of it having an effect.

The URL at the beginning of the run method is not helpful. There is no context explaining why it is relevant to this piece of code. The post asks about something that this code is not doing. The general consensus of the post is "there is no reason to do that and I have never seen that done."

public void run() {
    FileInputStream     fis = new FileInputStream(this.path);
    BufferedInputStream bis = new BufferedInputStream(fis);
    this.player             = new Player(bis);
    //...
}

By writing your class this way, it is always forced to read a file from disk. What happens if you want to stream the song over a socket? Passing the input stream in to the constructor (or even the Player instance) would make your class more flexible and easier to test.

while (!abort)
{
    if (playing)
        player.play(1);
}

If playing is set to false, this thread will spin and waste cpu time.

There is no reason Mp3PlayerThread to be a subclass of Thread. Making it a simple class that implements Runnable would allow you to use it in more contexts and not incur the heavy costs of creating a thread every time an instance is instantiated. An example of the expanded context would be to create a thread pool of size one. Then you can schedule multiple songs to be played just by adding an instance to the execution pool for each new song.

public Mp3Player(String filename)
{
    song = filename;
}

public Mp3PlayerThread(String file)
{
    this.path = file;
}

Why are the variable names changing when they are all the same thing? Variable shadowing is not a valid excuse as you can use this. or prefix your member variables with an underscore.

In general, you should decide if you want to use this. or not and be consistent.

Irritating answer.... it says pretty much everything I had in my answer, and more, so my answer is now moot.... +1 (and more if I could). — rolfl♦, 21 hours ago
Perhaps, so you may want to suggest a Conditoin as part of your answer. — rolfl♦, 21 hours ago
I have read your answer and it all makes perfect sense! :) I have modified my code and will update my question! Thank you very much! — Christophe De Troyer, 20 hours ago
@ChristopheDeTroyer - please don't edit changes back in to your question. Rather, if you want the changed code reviewed, ask a new question with the changed code. Read: What you can and cannot do after receiving answers — rolfl♦, 20 hours ago

vnp · Answer 2 · 2014-08-28 17:50:45Z

up vote 3 down vote

boolean playing doesn't have a right to exist (regardless of its volatility). Pausing the play results in a busy-wait: the thread consumes CPU as fast as possible doing just nothing. You need a condition variable instead of a simple flag.

answered 21 hours ago

vnp
4,884222

Hmm, I don't really understand what you are saying. But perhaps using a timer could be a solution? I could pause the timer instead? – Christophe De Troyer 21 hours ago

The paused thread shall suspend, and yield CPU indefinitely. Instead you have it constantly testing the flag. – vnp 21 hours ago

add a comment |

Christophe De Troyer · Answer 3 · 2014-08-28 19:21:17Z

Modifications per answers given

I have taken into account what was said in the answers of @unholysampler and @vnp and I have changed my code accordingly.

To stop the Mp3Thread from spinning and using CPU I used this.wait() which simply parks the thread. To make sure that I can reactivate the thread I need to call notify() on the lock object (being this). This can be done inside methods I implemented in the Mp3PlayerThread class itself to make sure we do not expose any unnecessary variables.
I have indeed abstracted way the stream and now the Mp3PlayerThread only uses BufferedInputStream.
Furthermore I have implemented the Runnable instead of using a Thread.

Thank you very much for your help!

A small note on the variable alignment: I use the OCDFormat plugin for that. However, your point is valid about the alignment.

Mp3PlayerThread

public class Mp3PlayerThread implements Runnable
{
    private volatile boolean playing = false;
    private volatile boolean abort = false;

    private Player player;
    private final BufferedInputStream songStream;

    public Mp3PlayerThread(BufferedInputStream songStream)
    {
        this.songStream = songStream;
    }

    @Override
    public void run()
    {
        synchronized (this)
        {
            try
            {
                // Initialize the player and set the play flag to true.
                this.player = new Player(songStream);
                this.playing = true;

                while (!player.isComplete() && !abort)
                {
                    while (!playing)
                        this.wait();

                    player.play(1);
                }
                Printer.debugMessage(this.getClass(), "end of file or stopped");

            } catch (JavaLayerException e)
            {
                e.printStackTrace();
            } catch (InterruptedException e)
            {
                e.printStackTrace();
            }
        }
    }
    /**
     * Pauses the playback of the thread.
     */
    public void pauseThread()
    {
        Printer.debugMessage(this.getClass(), "paused");
        this.playing = false;
    }
    /**
     * Resumes playback of thread (if possible).
     */
    public void resumeThread()
    {
        Printer.debugMessage(this.getClass(), "resuming");

        synchronized (this)
        {
            this.playing = true;
            notify();
        }
    }
    /**
     * Sets the abort flag to true. Doing so makes the run() method finish
     * and thus stopping the thread properly.
     */
    public void abortThread()
    {
        Printer.debugMessage(this.getClass(), "stopping");
        this.abort = true;
    }
}

Wrapping class Mp3Player

public class Mp3Player
{
    private Mp3PlayerThread playerThread;
    private BufferedInputStream songStream;

    public Mp3Player(BufferedInputStream songStream)
    {
        this.songStream = songStream;
    }

    /**
     * Initializes the playerThread with the given song, sets the flag to true
     * and starts playing the song.
     */
    public synchronized void play()
    {
        playerThread = new Mp3PlayerThread(songStream);
        new Thread(playerThread).start();
    }

    /**
     * Pauses the execution of the playerThread.
     */
    public synchronized void pause()
    {
        Printer.debugMessage(this.getClass(), "paused");
        playerThread.pauseThread();
    }

    public synchronized void resume()
    {
        synchronized (playerThread)
        {
            Printer.debugMessage(this.getClass(), "resuming");
            // playerThread.notify();
            playerThread.resumeThread();
        }
    }

    public synchronized void stop()
    {
        Printer.debugMessage(this.getClass(), "stopping");
        playerThread.abortThread();
        playerThread = null; // Needed or not?
    }
}

it won't pauze (the call gets blocked), fixed by putting the synchronized block around only the while(!playing)wait(); and to avoid repeatedly getting the lock putting the whole thing in a if(!playing) — ratchet freak, 16 hours ago
Well, in this case it does work because the pause method is not synchronized. I was wondering how to fix this but your tip might be helpful in making the control methods in the thread synchronized. — Christophe De Troyer, 16 hours ago

asked	today
viewed	230 times
active	today

current community

your communities

more stack exchange communities

Correct control of execution of Java thread

3 Answers 3

Your Answer

Not the answer you're looking for? Browse other questions tagged java multithreading or ask your own question.

Hot Network Questions

current community

your communities

more stack exchange communities

Correct control of execution of Java thread

3 Answers 3

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged java multithreading or ask your own question.

Related

Hot Network Questions