-1

I wanted to test out multithreading for a project of mine, trying to also develop a solution in case something goes wrong.

So I made this small test:

main

public class main
    {

        static int addToCounter;
        static int addToErrorCounter;
        public static void main(String[] args) throws InterruptedException
            {
                int threads = 10;
                Executor exec = new Executor();
                for (int i = 0; i < threads; i++)
                    {
                        double error = Math.random();
                        testClass aldo = new testClass();
                        Thread thread = aldo.getThread(300, error);
                        exec.execute(thread);
                    }
                while (threads != (addToCounter + addToErrorCounter))
                    {
                        System.out.println("Not all threads finished, number of finished threads is: " + (addToCounter + addToErrorCounter));
                        Thread.sleep(50);
                    }
                System.out.println("Number of Threads that finished correctly: " + addToCounter);
            }

    }

testClass

import test1.main;

public class testClass
    {

        public Thread getThread(long time, double error)
            {
                Thread thread = new Thread()
                    {

                        public void run()
                            {
                                try
                                    {
                                        Thread.sleep(time);
                                    }
                                catch (InterruptedException e)
                                    {
                                        // TODO Auto-generated catch block
                                        e.printStackTrace();
                                    }

                                if (error > 0.5)
                                    {
                                        main.addToErrorCounter++;
                                        throw new java.lang.Error("HELLO");
                                    }
                                System.out.println("I DID THIS!");
                                main.addToCounter++;
                            }
                    };

                return thread;

            }

    }

(you'll have to fix the imports, also I use a custom class Executor, although that's only a wrapper for ExecutorService)

The weird behaviour is that sometimes it works properly, and sometimes it doesn't (total terminated thread count is 9, although I can see clearly it printed "I DID THIS!" and the error exactly 10 times).

Any fix?

  • Why don't you let each thread also say it's name so you can keep up with which thread is starting/finishing as well as bumping down the thread count to something like 5 to help lower the count for troubleshooting? – Daniel Gale Apr 23 '18 at 14:47
  • What do you mean by saying it's name? The threads all use the same class and the only 'moving' part is the `Math.random()`. I don't get how saying the name would help here. – user9549355 Apr 23 '18 at 14:49
  • [What does your step debugger tell you?](http://stackoverflow.com/questions/25385173/what-is-a-debugger-and-how-can-it-help-me-diagnose-problems) this is easily understood with a single pass thorough your step debugger. –  Apr 23 '18 at 14:54
  • use `thread.setName("Thread" + i);` when setting the thread and then when you are printing, use getName() to show which thread is saying 'hello' or 'i did this'. You can determine what might be different if anything with the thread causing an issue. – Daniel Gale Apr 23 '18 at 14:56
  • 1
    Jarrod, how is this question a _duplicate_ of either of the questions you listed? Sure, the first is related, but a junior programmer just starting out in multi-threaded java is unlikely to identify that line as the problem code, and then to think to search around atomicity. I feel like you're giving the OP an excessively hard time given their apparent skill level. – user31601 Apr 23 '18 at 15:06

2 Answers2

0

The Problem might be a racecondition. the "++" operator is not atomic. Imageine the following scenario. There are two Threads at the same time. both want to increase a number and finish. The initial value of the number is 0. Thread 0 reads the number, knows now it is 0. Thread 1 reads the number, knows now it is 0. Thread 0 (who knew it was 0) now writes 1 to the memory. Thread 1 does not know, that the number has changed, and still believes the number is 0 so he also writes a 1 to the memory.

You need something like a synchronizing mechanisim, something like a lock, or a semaphore or something else.

have a look at this for more information: http://winterbe.com/posts/2015/04/30/java8-concurrency-tutorial-synchronized-locks-examples/

for your example you could use the "synchronized" example from that link.

add a method to your main class looking like this to increment the addToCounter and also to the addToErrorCounterto remove the effects from your error counter:

synchronized AddToError(int e){
    addToError += e;
}
synchronized IncCounter(){
    addToCounter++;
}

call those methods in your threads in the testclass instead of incrementing them unsynchronized.

Michriko
  • 337
  • 1
  • 11
0

My guess is that the postfix operator (main.addToCounter++) is not atomic. This line of code is probably equivalent to something like:

int temp = main.addToCounter;
main.addToCounter = temp + 1;
return temp;

With multiple threads doin this at the same time, two threads could obtain the same value for temp (because both peform the first line in the above pseudo-code before either performs the second), and hence the counter total will be too small once all threads are complete. See Why is i++ not atomic? for more information.

A quick fix in this situation is to make addToCounter an AtomicInteger, then use addToCounter.incrementAndGet() in place of addToCounter++.

user31601
  • 2,482
  • 1
  • 12
  • 22