HashMap synchronization for Map with value-incrementation

Question

I have a questions regarding synchronization of HashMap. The background is that I am trying to implement a simple way of Brute-Force-Detection. I will use a map which has username as key and is used to save the amount of failed login attempts of the user. If a 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 I have in mind at the moment is pretty simple: I would just track this for the whole 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 schronized Map (Collections.sychronizedMap()) or is a ConcurrentHashMap sufficient? Or maybe even just a normal HashMap? I guess it's not that much of a problem if a few increments slipped through?

nwinkler · Accepted Answer · 2012-03-02T14:46:13.930

5

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 with the comparison, but it will help you with keeping numbers accurate - no need to doing the ++ and the put in two steps.

On the 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
}

edited Mar 02 '12 at 14:46

answered Mar 02 '12 at 14:21

nwinkler

52,665
21
154
168

You still need a synchronized block around that. – aioobe Mar 02 '12 at 16:42
Wooops. Didn't see the putIfAbsent :) really elegant solution :) +1! – aioobe Mar 02 '12 at 19:42
That was really helpful! Didn't know AtomicInteger, what a great idea to make "increment" and "get" one atomic operation. Thank you! – Mario B Mar 03 '12 at 09:20
1

If myMap didn't contain a mapping for "username" then you get NullPointerException – Oleksandr Bondarenko Aug 06 '14 at 10:29

score 3 · Answer 2 · answered Mar 02 '12 at 14:18

Unless you synchronize the whole block of code that does the update, it won't work as you expect anyway.

A synchronized map just makes sure that nothing nasty happens if you call, say, put several times simultaneously. It doesn't make sure that

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

is executed atomically.

You should really synchronize the whole block that performs the update. Either by using some Semaphore or by using the synchronized keyword. For example:

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
    }
}

score 2 · Answer 3 · answered Mar 02 '12 at 14:29

The best way I see would be to store the fail counter with the user object, not in some kind of global map. That way the synchronization issue does not even turn up.

If you still want to go with the map you can get aways 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 optimization attempt here is a waste of time. Its not like your system will process millions of login failures a second, so your orginal code should do just fine.

+1 You can make FailCount an AtomicInteger to make it thread safe. — Peter Lawrey, Mar 02 '12 at 15:00

score 1 · Answer 4 · answered Mar 02 '12 at 14:17

1

The simplest solution I see here is to extract this code into a separate function and make in synchronized. (or put all your code into a synchronized block). All other left unchanged. Map variable should be made final.

answered Mar 02 '12 at 14:17

dhblah

9,751
12
56
92

score 1 · Answer 5 · answered Mar 02 '12 at 14:21

Using a synchronized HashMap or a ConcurrentHashMap is only necessary if your monitoring application is multi-threaded. If that is the case, ConcurrentHashMap has significantly better performance under cases of high load/contention.

I would not dare use an unsynchronized structure with multiple threads if even one writer/updater thread exists. It's not just a matter of losing a few increments - the internal structure of the HashMap itself could be corrupted.

That said, if you want to ensure that no increment is lost, then even a synchronized Map is not enough:

UserX attempts to login
Thread A gets count N for "UserX"
UserX attempts to login again
Thread B gets count N for "UserX"
A puts N + 1 to the map
B puts N + 1 to the map
The map now contains N + 1 instead of N + 2

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

HashMap synchronization for Map with value-incrementation

5 Answers5