Java - multithreading increment

Question

I have the following class in my application

public class InsertErrorLinesHandler {

    private int currentCount;

    public void insertErrorLine() {
        //do something...
        currentCount++;
    }
}

And I have multiple threads that are using the same instance of InsertErrorLinesHandler, particulary calling insertErrorLine method. After all these threads are stopped, I get the currentCount from this instance.

The question is - how to rewrite this class to be sure that there won't be any concurrency problems? What I want is to be sure, that currentCount value will be the count of method callings from threads. Should I use static method and static variable? Make method synchronize? Make variable volatile?

Thanks!

Timetable

Answer 1

I suggest using an AtomicInteger, which has a thread-safe increment method

Astyanax

Brian Goetz's book "Java Concurrency in Practice" specifically addresses the issue of keeping a counter that's accessible from multiple threads. His solution is to use an AtomicLong. Simple, and effective. – Argumentative

Answer 2

Simple fix, make the method call synchronized:

public class InsertErrorLinesHandler {

    private int currentCount;

    public void synchronized insertErrorLine() {
        //do something...
        currentCount++;
    }
}

Verdellverderer

That is how I'd like to do it, but there was doubts, maybe there's a better way... – Timetable

The best way is usually the simplest. Other methods will involve having a static object which is used to synchronize the increment operation, importing several classes, or even rewriting the class. Choose wisely – Verdellverderer

Simple, yes; but very inefficient if //do something... takes significant time and does not impact other threads. Less simple, but more optimal would be to wrap a synchronized block around the currentCount++ statement. – Profanity

It amuses me to see that your simple fix contains a huge bug. Without marking the field as volatile, other threads may fail to see a change. Synchronized alone does not make things safe for multithreading. – Leahy

@NicoVanBelle, I see your point about making it volatile, but is that implying that after a thread enters the synchronized block and does currentCount++, there is a chance that the actual value of currentCount would not have been written to memory? – Verdellverderer

You may ignore my previous comment as I indeed thought a modification would be written to the CPU cache. I always remembered visibility was obtained by using volatile explicitly but the usage of synchronized alone provides the same visibility! – Leahy

@NicoVanBelle, you raised an interesting point, so I'm still going to experiment a bit with this and see if I can potentially reproduce the bug you described. I will update this thread with my results – Verdellverderer

Answer 3

Use AtomicInteger or use LongAdder when number of threads are more for better performance.

Here is the example on how to use AtomicInteger,

public class InsertErrorLinesHandler {
   AtomicInteger counter = new AtomicInteger();

   public void insertErrorLine() {
       counter.incrementAndGet();
   }

   public int get() {
       return counter.get();
   }
}

Consalve

Answer 4

So far, nobody's addressed this part of your question:

Should I use static...?

The choice of whether or not to use static is completely independent of the choice of whether to use synchronized or AtomicInteger or not. The answer depends on what you want to count.

A static field in Java is what some other languages call a global variable: There is only one of it for the entire program. A non-static field (a.k.a., an instance variable) exists in multiple copies---one for each instance of the class to which it belongs.

How many instances of the InsertErrorLineHandler class does your program create? If more than one, then do you want each instance to have its own counter, or do you want all of them to share the same counter? Declaring the field static means that they will all share the same, and leaving out the static keyword means that each instance will have its own counter.

If your program only ever creates one InsertErrorLineHandler instance (i.e., if you are using it as a singleton class) then you should not use static. Making fields static or not static won't change the behavior of a singleton, but using static in a singleton would be bad style.

Profanity

Only one instance of InsertErrorLineHandler. So I think it was right that I didn't use static – Timetable