rspec/rules/S2180/java/rule.adoc

== Why is this an issue?

Concurrent maps are used for thread-safety, but the use of such maps alone does not ensure thread-safety; they must also be _used_ in a thread-safe manner. Specifically, retrieving a key's value from a map, and then using ``++put++`` to add a map element if the value is ``++null++`` is not performed in an atomic manner. Here's what can happen

----
Thread1 cmap.get("key") => null
Thread2 cmap.get("key") => null
Thread1 cmap.put("key", new Value())
Thread2 cmap.put("key", new Value())
----
Note that this example is written as though the threads take turns performing operations, but that's not necessarily the case.


Instead of ``++put++``, ``++putIfAbsent++`` should be used. 


=== Noncompliant code example

[source,java]
----
private static final ConcurrentMap<String,MyClass> cmap = new ConcurrentHashMap<String,MyClass>();

public void populateMyClass(String key, String mcProp) {
  MyClass mc = cmap.get(key);
  if (mc == null) {
    mc = new MyClass();
    cmap.put(key, mc);  // Noncompliant
  }
  mc.setProp(mcProp);  // could be futile since mc may have been replaced in another thread!
}
----


=== Compliant solution

[source,java]
----
private static final ConcurrentMap<String,MyClass> cmap = new ConcurrentHashMap<String,MyClass>();

public void populateMyClass(String key, String mcProp) {
  MyClass mc = cmap.get(key);
  if (mc == null) {
    mc = new MyClass();
    cmap.putIfAbsent(key, mc);
    mc = cmap.get(key); // re-retrieve value since another thread could have beaten this one to the "put"
  }
  mc.setProp(mcProp);
}
----


== Resources

* https://wiki.sei.cmu.edu/confluence/x/8jdGBQ[CERT, VNA03-J.] - Do not assume that a group of calls to independently atomic methods is atomic


ifdef::env-github,rspecator-view[]

'''
== Implementation Specification
(visible only on this page)

=== Message

Use "putIfAbsent" instead of "put".


'''
== Comments And Links
(visible only on this page)

=== on 22 Oct 2014, 14:56:53 Ann Campbell wrote:
\[~nicolas.peru] note that in the code sample I also demonstrate that you must re-retrieve the value from the map before modifications. I think re-retrieval is also worthy of checking, but I don't know if you'd want to include it in this rule or have a separate rule for that.

=== on 27 Feb 2015, 10:49:19 Nicolas Peru wrote:
Your question shall be answered by implementer.

endif::env-github,rspecator-view[]