Thread won't die and two of them keep overlapping

Question

I need a thread that will only run once at a time, for example if it's called for the first time it will run, if it is called a second time, the first should stop completely and be allowed to die and a new one should take it's place. I was ran a small test to see what was actually happening between each execution, the results show that the thread doesnt die but instead two threads are being executed alongside:

public class Test {
    Worker worker = new Worker();

    @Override
    public void valid() {

        try {
            if (worker.running) {
                worker.running = false;
                worker.join();
            }

        } catch (InterruptedException iex) {
            worker.running = false;
        }

        worker = new Worker();

        worker.start();

    }

    private final class Worker extends Thread {
        private volatile boolean running = true;

        @Override
        public void run() {
            while (running) {
                System.out.println(Thread.currentThread().getName());
                try {
                    Thread.sleep(2000);
                } catch (InterruptedException iex) {
                    Thread.currentThread().interrupt();
                }
            }
        }
    }
}

The results are as follows:

//Upon first execution
Thread-4
Thread-4
Thread-4
Thread-4

//When I execute it again
Thread-7
Thread-4
Thread-7
Thread-4
Thread-7
Thread-4

I've tried using ExecutorService or using while(!Thread.currentThread.isInterrupted) instead of the boolean flag, and got the same results. How can I properly stop "Thread-4" and have only one of them running?

The actual issue comes from a thread that will cycle through a list and update things on discord chat by request, what the thread does is listen to input and change as suggested by kidney I'm trying to use executor.submit() and Future

private ExecutorService executor = Executors.newSingleThreadExecutor();

    private Future<Void> worker;

    private void setupImageThread() {

        if (!worker.isDone() && !worker.isCancelled()) {
            worker.cancel(true);
        }

        this.worker = (Future<Void>)executor.submit(new Cycler(Listener.queue(), this.links, Cel.cMember()));

        ScheduledExecutorService ses = Executors.newScheduledThreadPool(1);

        Runnable timeout = () -> {

            executor.shutdown();

        };

        ses.schedule(timeout, 100, TimeUnit.SECONDS);
    }

How can I go about initializing the Future for the first time it is created?

Answer 1

Using single thread executor service, I would try something like this:

public class Test {

    private static ExecutorService executor = Executors.newSingleThreadExecutor();

    Future<Void> worker;

    public Test() {
        this.worker = executor.submit(new Worker());
    }

    @Override
    public void valid() {
        if (!worker.isDone() && !worker.isCancelled()) {
            worker.cancel(true);  // Depends on whether you want to interrupt or not
        }

        this.worker = executor.submit(new Worker());
    }
}

And make Worker implement Runnable.

Answer 2

It seems that the method valid can be called several times simultaneously. That means, every of those calls will wait to end only for one thread (Worker), whereas, every of them creates its own Worker and you lose a pointer to it, so it impossible to stop bunch of new created workers. You should make the valid method synchronized: synchronized void valid() it will prevent creating many workers:

@Override
synchronized public void valid() {
    ...
}

One more thing to say. You put the while loop outside the try-catch, which is wrong: if the tread gets interrupted, the interruption doesn't kill it, because next interation gets started, so it should be like that:

@Override
public void run() {
    try {
        while (running) {
            System.out.println(Thread.currentThread().getName());
            Thread.sleep(2000);
        }
    catch (InterruptedException iex) {
       //you don't need here Thread.currentThread().interrupt() call, because the thread has alredy been interrupted. 
       // The return statement here is also obsolete, I just use it as an example, but you can use empty braces.
       return; 
    }
    }
}