ConcurrentHashMap with immutable values - synchronized replace?

Question

Im using a ConcurrentHashMap in a multi-threaded program. The map maps ServerIDs to objects containing more information about the server (whether it is online or not, how much it has been used recently, etc.). Both the ServerID and the ServerInformation are immutable.

To update the server information I am doing more or less what is proposed as point B) in this question: What is the preferred way to modify a value in ConcurrentHashMap?

namely (modified to use my own variable names) this:

public void addUsage(ServerID id, long moreUsage) {
    ServerInfo oldInfo = serverMap.get(id);
    ServerInfo newInfo = oldInfo.addUsage(moreUsage);
    serverMap.put(id, newInfo);
}

Now my question is: Shouldn't this method be synchronized to rule out the possibility of lost updates?

Or is there another way to achieve that? Maybe something like the following (edited from my original version to remove an obvious mistake):

public void addUsage(ServerID id, long moreUsage) {
    ServerInfo oldInfo = serverMap.get(id);
    ServerInfo newInfo = oldInfo.addUsage(moreUsage);
    while (!serverMap.replace(id, oldInfo, newInfo) ) {
        oldInfo = serverMap.get(id);
        newInfo = oldInfo.addUsage(moreUsage);
        // try again later
        Thread.sleep(SOME_TIME);
    };
}

`Thread.sleep` is the wrong response to a failed `replace`; instead, you should redo the computation: redo `serverMap.get(id)`, and redo `addUsage`. — Louis Wasserman, Nov 08 '13 at 21:45
You are, of course, right! Otherwise the replace would probably never again be able to succeed... Does it make sense to modify my question to reflect your suggestion? Sorry, I am new here... — david.mihola, Nov 08 '13 at 21:48

score 2 · Accepted Answer · answered Nov 08 '13 at 21:45

Yes, the replace method is the right way to solve this problem. Just remember to override equals on your value objects appropriately!

(This assumes calculating newValue is relatively cheap and collisions are relatively rare. If it's an intensive calculation and collisions are common, it may be worthwhile to introduce a mutex and serialize the calculations.)

It is probably better to re-submit the job to whatever queue is feeding your threads, or put it into a delayed queue, rather than actually put a worker thread to sleep. Sleeping worker threads is overall sad and lacking in scalability.

ConcurrentHashMap with immutable values - synchronized replace?

1 Answers1