java:S1860 false positive: Synchronize on a new "Object" instead

Rules and Languages Report False-positive / False-negative...
,
1
  • versions used (SonarQube, Scanner, language analyzer)
    SonarQube Version: 8.9.2.46101

The detection of “Synchronization should not be done on instances of value-based classes” in wrong. Following code, pool.intern(applyOid.toString()) is an object of a new thread-safe interner, it’s not value-based classes.

import com.google.common.collect.Interners;
// ...ignore codes
private Interner<String> pool = Interners.newStrongInterner();
// ...ignore codes
public CustomResult<DataSetDTO> saveDatasetToDraft(@PathVariable("applyOid") GUID applyOid, @RequestBody DataSetDTO data) {
	// report java:S1860, Synchronize on a new "Object" instead.
    synchronized (pool.intern(applyOid.toString())) {
        DataSetDTO result = dataSetApplyService.save(data, getOperator());
        return CustomResult.result(true, result);
    }
}
4

Hi Martin, sorry for the very late reply.

Reading the documentation of the guava Interner it looks to me like the behavior is similar to the java.lang.String.intern, which relies on pooling values and returning references to the same object in memory.

The JavaDocs of Google Interner states:

intern(a).equals(a) always holds, and intern(a) == intern(b) if and only if a.equals(b).

This means that if you intern twice the same string literal, you will get 2 references to the same object. This can compromise the way the JVM decides to lock and release the monitor of the String you are synchronizing on and can lead to deadlocks.

I think that as a rule of thumb, any kind of pooling mechanism combined with synchronization is generally advised against.

Edit:
To elaborate further, I realize that in the case of very complex and unique strings, like GUIDs, user tokens, and so on, you might want to intern them exactly to have a lock on that unique identifier, but in most cases, it would be very hard for the Java analyzer to distinguish between unique strings and common ones.

Let me know if it makes sense to you,
thanks!