0

So, basically, my code currently is just taking in one argument from the commmand line, which is the folder in which all of the wave files are to play in the playlist. I have another thread which takes in user input. When the user inputs 'skip' it will continue through the for loop of items. It all works fine, but, if i type skip, and hit enter, sometimes it skips twice. I presume it has something to do with the threading business. Here's my code.

 package com.thechief.music;

 import java.io.File;
 import java.io.FilenameFilter;
 import java.util.Scanner;

 import javax.sound.sampled.AudioInputStream;
 import javax.sound.sampled.AudioSystem;
 import javax.sound.sampled.Clip;

 public class Player {

public static int index = 0; // The current clip playing
public static Clip[] clip;

public static void main(String[] args) throws Exception {
    Scanner scanner = new Scanner(System.in);

    FilenameFilter textFilter = new FilenameFilter() {
        public boolean accept(File dir, String name) {
            return name.toLowerCase().endsWith(".wav");
        }
    };

    File[] files = new File(args[0]).listFiles(textFilter);
    clip = new Clip[files.length];

    Runnable b = new Runnable() {
        public void run() {
            while (true) {
                String command = scanner.next();
                if (command.equals("skip")) {
                    getCurrentClip().stop();
                } else if (command.equals("back")) {
                    getCurrentClip().stop();
                    if (index > 0) {
                        index -= 2;
                    } else {
                        System.out.println("Cannot go back further than the first item.");
                    }
                }
            }
        }
    };
    Thread second = new Thread(b);
    second.start();

    for (index = 0; index < clip.length; index++) {
        if (index < 0) index = 0;
        clip[index] = AudioSystem.getClip();
        if (index < 0) index = 0;
        AudioInputStream ais = AudioSystem.getAudioInputStream(files[index]);
        if (!getCurrentClip().isOpen()) {
            getCurrentClip().open(ais);
        } else {
            getCurrentClip().close();
            getCurrentClip().open(ais);
        }

        getCurrentClip().start();

        System.out.println("Now Playing: " + files[index].getName() + ", Time Left: "
                + (getCurrentClip().getMicrosecondLength() / 1000000.f) + " seconds.");

        while (getCurrentClip().getMicrosecondLength() != getCurrentClip().getMicrosecondPosition()
                || getCurrentClip().isActive()) {
            if (!getCurrentClip().isActive()) {
                break;
            }
        }
    }

    System.out.println("Playlist completed.");

    scanner.close();
    second.join();
}

public static Clip getCurrentClip() {
    return clip[index];
}
 }
Ameer Ali
  • 3
  • 1

1 Answers1

0

Here:

public static int index = 0; // The current clip playing

That index field is used and updated by more than one thread. That means: anything can happen.

A first step: use AtomicInteger instead of int here.

And the real answer of course: you have to research what you are doing. Just adding new Thread().start() here or there without knowing and understanding what you are doing is a nothing but a recipe to run into all sorts of unpredictable behavior. It is super simple: when mutable data is shared between multiple threads, then you absolutely must ensure that the necessary precautions are in place.

To give a bit more guidance: to prevent things like if (index < 0) index = 0; to enable races.

Meaning: one thread sees index=1 for example, and then turns index to 0. But at the same time, another thread tried to read index, and used wrong intermediate content.

When you turn that index into an AtomicInteger, you can start doing things like:

synchronized(index) {
  if (index ...
    index = ...
}

for example.

GhostCat
  • 137,827
  • 25
  • 176
  • 248
  • Thanks for such a quick reply! I'll try it out and see if it works. – Ameer Ali Nov 04 '18 at 17:15
  • Hmm... It doesn't seem to be working. Maybe because I'm not too familiar with AtmoicInteger. Basically what I did was just replace int with AtomicInteger, and replaced all of the functionality of 'index' with its AtomicInteger counterpart, but there is no difference. Sorry for sounding a bit dumb if there is something simple that I'm doing wrong :p – Ameer Ali Nov 04 '18 at 17:40
  • Oh and I've tried to add the 'volatile' keyword to `index` and `clip` that didn't work as well. – Ameer Ali Nov 04 '18 at 17:42
  • @AmeerAli See my updates. Please note: I can't rework all your code, I am trying to give you guidance here. The first thing for you is to understand the "events" that change your shared data. Then you have to make sure that all threads only see a **valid** data point, and that there are no race conditions. – GhostCat Nov 04 '18 at 17:47