CODEWITHSUNDEEP
javamultithreading
user1849859
user1849859

Reputation: 319

While loop with threads does not stop

I am using a while loop to know when to stop the application and I'm using an executor service for threads:

ExecutorService executorService = Executors.newFixedThreadPool(8);
String[] error = {""};
while(!(error[0].equals("Nothing found"))) {
    executorService.execute(new Runnable() {
        @Override
        public void run() {
            addProductsToSet();

            //rest of the code
            synchronized (error[0]) {
                error[0] = // code that might return "Nothing found" to stop the while loop
            }
            //rest of the code
        } //end of run method
    });
}

executorService.shutdown();
while(!executorService.isTerminated()) {
}

The problem that I have is that error[0] has the value "Nothing found" but the code just doesn't stop. It goes and goes on forever. If I'm not using threads, the while loop stops. I also tried executorService.shutdownNow() but doesn't work.

Any suggestions?

Upvotes: 0

Views: 1484

Answers (3)

Areo
Areo

Reputation: 21

The whole pattern looks kind of flawed. I would suggest a Pattern like this:

ExecutorService executorService = Executors.newFixedThreadPool(4);
executorService.execute(new Runnable() {
    @Override
    public void run() {
        while(true) {
            addProductsToSet();
            // rest of the code

            String error = code that might return "Nothing found" to stop the while loop
            if(error.equals("Nothing found")) {
                break;
            }
        }
        // rest of the code
    } // end of run method
});

executorService.shutdown();
while (!executorService.isTerminated()) {
}

With this, each Thread stops if the method returns "Nothing found" and you don't start up a new Threadpool in each while-loop iteration.

Upvotes: 0

Tagir Valeev
Tagir Valeev

Reputation: 100209

There are two problems in your code. First, you are relying that non-volatile variable write to error[0] will be visible from another thread. You cannot rely on it. JVM may optimize your code to read the error[0] only once as it does not guarantee that the writes in another thread will be visible. Note that array elements are always non-volatile, so you cannot do it correctly with an array. The correct way would be for example to use AtomicReference:

AtomicReference<String> error = new AtomicReference<>("");
while (!(error.get().equals("Nothing found"))) {
    executorService.execute(new Runnable() {
        @Override
        public void run() {
            System.out.println(i.incrementAndGet());
            addProductsToSet();
            // rest of the code

            error.set("Nothing found");
            // rest of the code
        } // end of run method
    });
}

Though probably if you have only two states (empty string and "Nothing found"), then AtomicBoolean would be the best solution. And, of course, you should not synchronize on AtomicReference.

The second problem is that you create an enormous number of tasks before they will even start to execute. Your while loop just submits tasks to the pool without any waiting, so when at least one task will be actually launched and able to say that "Nothing found" you will have thousands of tasks in the pool and you will have to wait till all of them finish in shutdown(). You may fix this quickly by changing shutdown() to shutdownNow(), but I guess this will just hide the original problem. The actual thing you have to do is reconsider whether you need such many tasks.

Upvotes: 1

Neil Coffey
Neil Coffey

Reputation: 21795

One fundamental problem you have with your code is that you are trying to synchronise on "whatever object is the 0th element of the array", but that element itself may change. The other problem is that you're not synchronising on anything at all when you try to read the state of the error.

If you really must use a String as a marker of whether to continue in this way, then solutions are to use a simple String variable, but declare it as volatile, or take a look at the AtomicReference class.

That said, I would strongly suggest looking at trying to change your program flow here to use the Callable interface rather than Runnable. You pass a Callable into Executor.submit() method, and effectively what this allows you to do is for the task in the thread to "properly" pass back an error (as an exception) to the posting thread (the thread running your loop in this case). That way, you will avoid potential synchronisation errors and your program flow will be much clearer, I think.

Upvotes: 1

Related Questions