StackOverflow2013

Note that there are some explanatory texts on larger screens.

plurals

PO
text
Body
copied!<p>There is the <em>potential</em> for a subtle bug in your code. </p> <p>[<strong>UPDATE:</strong> Since he's using map.remove() this description isn't totally valid. I missed that fact the first time thru. :( Thanks to the question's author for pointing that out. I'm leaving the rest as is, but changed the lead statement to say there is <em>potentially</em> a bug.]</p> <p>In <strong>doWork()</strong> you get the List value from the Map in a thread-safe way. Afterward, however, you are accessing that list in an unsafe matter. For instance, one thread may be using the list in <strong>doWork()</strong> while another thread invokes <strong>synchronizedMap.get(key).add(value)</strong> in <strong>addToMap()</strong>. Those two access are not synchronized. The rule of thumb is that a collection's thread-safe guarantees don't extend to the keys or values they store. </p> <p>You could fix this by inserting a synchronized list into the map like</p> <pre><code>List<String> valuesList = new ArrayList<String>(); valuesList.add(value); synchronizedMap.put(key, Collections.synchronizedList(valuesList)); // sync'd list </code></pre> <p>Alternatively you could synchronize on the map while you access the list in <strong>doWork()</strong>:</p> <pre><code> public void doWork(String key) { List<String> values = null; while ((values = synchronizedMap.remove(key)) != null) { synchronized (synchronizedMap) { //do something with values } } } </code></pre> <p>The last option will limit concurrency a bit, but is somewhat clearer IMO.</p> <p>Also, a quick note about ConcurrentHashMap. This is a really useful class, but is not always an appropriate replacement for synchronized HashMaps. Quoting from its Javadocs,</p> <blockquote> <p>This class is fully interoperable with Hashtable in programs that rely on its thread safety <em>but not on its synchronization details</em>.</p> </blockquote> <p>In other words, putIfAbsent() is great for atomic inserts but does not guarantee other parts of the map won't change during that call; it guarantees only atomicity. In your sample program, you are relying on the synchronization details of (a synchronized) HashMap for things other than put()s.</p> <p>Last thing. :) This great quote from <em>Java Concurrency in Practice</em> always helps me in designing an debugging multi-threaded programs.</p> <blockquote> <p>For each mutable state variable that may be accessed by more than one thread, all accesses to that variable must be performed with the same lock held.</p> </blockquote>

Querying!

Guidance

An individual column

Larger individual text columns get their own page to allow for proper reading.

SQuiL has stopped working due to an internal error.

If you are curious you may find further information in the browser console, which is accessible through the devtools (F12).

Reload