2

We have a web application which receives some million requests per day, we audit the request counts and response status using an interceptor, which intern calls a class annotated with @Async annotation of spring, this class basically adds them to a map and persists the map after a configured interval.
As we have fixed set of api we maintain ConcurrentHashMap map having API name as key and its count and response status object as value.So for every request for an api we check whether it exists in our map , if exist we fetch the object against it otherwise we create an object and put it in map. For ex

class Audit{
    CounterObject =null;
    if(APIMap.contains(apiname){
         // fetch existing object  
          CounterObject=APIMap.get(apiname);
    }
    else{
          //create new object and put it to the map
          CounterObject=new CounterObject();
    }
  // Increment count,note response status and other operations of the CounterObject recieved
}

Then we perform some calculation on the received object (whether from map or newly created) and update counters.
We aggreagate the map values for specific interval and commit it to database.
This works fine for less hits , but under a high load we face some issues. Like
1. First thread got the object and updated the count, but before updating second thread comes and gets the value which is not the latest one, by this time first thread has done the changes and commits the value , but the second threads updates the values it got previously and updated them. But as the key on which operation is performed is same for both the threads the counter is overwritten by the thread whichever writes last.
2. I don't want to put synchronized keyword over the block which has logic for updating the counter. As even if the processing is async and the user gets response even before we check apiname in map still the application resources consumed will be higher under high load if synchronized keyword is used , which can result in late response or in worst case a deadlock.



Can anyone suggest a solution which does can update the counters in concurrent way without having to use synchronized keyword.
Note :: I am already using ConcurrentHashMap but as the lock hold and release is so fast at high load by multiple threads , the counter mismatches.

Anand Kadhi
  • 1,790
  • 4
  • 27
  • 40
  • 1
    If the purpose is just to manage the request count then we can use atomic integer. See here http://stackoverflow.com/questions/4818699/practical-uses-for-atomicinteger. Also, I think having synchronized won't do any harm as the logic itself is enclosed in async block, hence, delay in this calculation won't cause the delay in response. – Darshan Mehta Dec 18 '15 at 17:05
  • @DarshanMehta: I have already mentioned in the question that after the response this processing is done but it would still consume the cpu resources if synchronized is used thus indirectly causing delay in response. – Anand Kadhi Dec 18 '15 at 18:24

2 Answers2

1

In your case you are right to look at a solution without locking (or at least with very local locking). And as long as you do simple operations you should be able to pull this off.

First of all you have to make sure you only make one new CounterObject, instead of having multiple threads create one of their own and the last one overwriting earlier object.

ConcurrentHashMap has a very useful function for this: putIfAbsent. It will story an object if there is none and return the object that is in the map right after calling it (although the documentation doesn't state it as directly, the code example does). It works as follows:

CounterObject counter = APIMap.putIfAbsent("key", new CounterObject());
counter.countStuff();

The downside of the above is that you always create a new CounterObject, which might be expensive. If that is the case you can use the Java 8 computeIfAbsent which will only call a lambda to create the object if there is nothing associated with the key.

Finally you have to make sure you CounterObject is threadsafe, preferably without locking/sychronization (although if you have very many CounterObjects, locking on it will be less bad than locking the full map, because fewer threads will try to lock the same object at the same time).

In order to make CounterObject safe without locking, you can look into classes such as AtomicInteger which can do many simple operations without locking.

Note that whenever I say locking here it means either with an explicit lock class or by using synchronize.

Thirler
  • 20,239
  • 14
  • 63
  • 92
  • Thanks , actually i am already using AtomicLong for counting the requests but still i am facing this issue as the CounterObject "has-a" relationship with counters and the object itself is not using any synchronization ,i will try soultion suggested by you. – Anand Kadhi Dec 18 '15 at 17:16
  • The relationship in itself from CounterObject to the counters is no problem. The problem is if you create new objects (without thinking about threadsafety) and if you do operations that depend on reading a value and then doing a computation and then storing it. If you want to know more about what you can and can't do read up on 'race conditions' and read Java Concurrency in Practice which is a great book on everything that has to do with multithreading in java – Thirler Dec 20 '15 at 09:30
  • Actually i am noting down request per API so in case i have 10 Api's , i make 10 counterObjects and note their requests by ConcurrentHashMap – Anand Kadhi Dec 21 '15 at 05:39
  • As long as you only increment the counters to count the number of requests it should be no problem. – Thirler Dec 21 '15 at 07:37
0

The reason for counter mismatch is check and put operation in the Audit class is not atomic on ConcurrentHashMap. You need to use putIfAbsent method that performs check and put operation atomically. Refer ConcurrentHashMap javadoc for putIfAbsent method.

  • Forgot to mention in the question, i am already using AtomicLong but the counter is mismatching as the object "has-a" counter and the object itself is not using any synchronization. – Anand Kadhi Dec 18 '15 at 17:18
  • 1
    AtomicLong is helping in incrementing the counter atomically. But the atomicity is missing while putting the CounterObject in the ConcurrentHashMap. It could happen that more than one thread finds that CounterObject is not present for a particular same key in ConcurrentHashMap and each of them creates a new CounterObject with initial value (1) and puts that in ConcurrentHashMap against same key. This results each thread replacing one another effectively resulting in count mismatch. To prevent this putIfAbsent is helpful. – Madhusudana Reddy Sunnapu Dec 18 '15 at 17:37