HashMap synchronization for map with value-incrementation

Question

I have questions regarding HashMap synchronization. The background is that I'm trying to implement a simple Brute-Force-Detection method. I will use a card that has a username as a key and is used to save the number of failed user login attempts. If the login fails, I want to do something like this:

Integer failedAmount = myMap.get("username"); if (failedAmount == null) { myMap.put("username", 1); } else { failedAmount++; if (failedAmount >= THRESHOLD) { // possible brute force detected! alert admin / slow down login // / or whatever } myMap.put("username", failedAmount); } 

The mechanism that I have in mind at the moment is pretty simple: I would just keep track of this all day and clear the () HashMap at midnight or something like that.

so my question is: What is the best / fastest map implementation I can use for this? Do I need a fully synchronized map (Collections. PsychronizedMap ()) or enough ConcurrentHashMap? Or maybe even a regular HashMap? I assume this is not so much if several increments slip through?

Answer 1

I would use a combination of ConcurrentHashMap and AtomicInteger http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/atomic/AtomicInteger.html .

Using AtomicInteger will not help you in comparison, but it will help you with the exact preservation of numbers - no need to do ++ and put in two steps.

In ConcurrentHashMap I would use the putIfAbsent method, which will eliminate your first if condition.

 AtomicInteger failedAmount = new AtomicInteger(0); failedAmount = myMap.putIfAbsent("username", failedAmount); if (failedAmount.incrementAndGet() >= THRESHOLD) { // possible brute force detected! alert admin / slow down login // / or whatever } 

Answer 2

If you do not synchronize the entire block of code that performs the update, it will not work as you expect.

A synchronized card simply ensures that nothing unpleasant happens if you call, say, put several times at the same time. He does not guarantee that

 myMap.put("username", myMap.get("username") + 1); 

performed atomically.

You must truly synchronize the entire block that performs the update. Either using Semaphore , or using the synchronized . For instance:

 final Object lock = new Object(); ... synchronized(lock) { if (!myMap.containsKey(username)) myMap.put(username, 0); myMap.put(username, myMap.get(username) + 1); if (myMap.get(username) >= THRESHOLD) { // possible brute force detected! alert admin / slow down login } } 

Answer 3

The best way that I see is to store a failure counter with a user object, and not in any global map. Therefore, the synchronization problem does not appear.

If you still want to go with the map, you can access with a partially synchronized approach if you use a mutable counter object:

 static class FailCount { public int count; } // increment counter for user FailCount count; synchronized (lock) { count = theMap.get(user); if (count == null) { count = new FailCount(); theMap.put(user, count); } } count.count++; 

But, most likely, any attempt to optimize here is a waste of time. It's not like your system will handle millions of failed login attempts, so your original code should only do well.

Answer 4

The simplest solution that I see here is to extract this code into a separate function and make it synchronized. (or put all your code in a synchronized block). All the rest remained unchanged. The map variable must be final.

Answer 5

Using a synchronized HashMap or ConcurrentHashMap is only necessary if your monitoring application is multithreaded. If so, ConcurrentHashMap has significantly better performance in high load / competition cases.

I would not dare to use an unsynchronized structure with multiple threads if at least one writer / updater thread exists. This is not just a loss of a few steps - the internal structure of the HashMap itself can be damaged.

However, if you want the increment not to be lost, then even a synchronized Map not enough:

• UserX is trying to log in
• Thread A Gets Score N For "UserX"
• UserX tries to log in again
• Thread B gets score N for "UserX"
• A puts N + 1 on the card
• B puts N + 1 on the card
• Now the map contains N + 1 instead of N + 2

To avoid this, use either a synchronized block for the entire get / set operation, or use something along the AtomicInterer lines instead of a simple Integer for your counter.