1

my code:

    private volatile String tokenCache;
    private volatile long tokenLastUpdateTimeStamp;

    private String getToken() throws IOException {
        // cache in 10 minutes.
        if (StringUtils.isNotEmpty(tokenCache) && System.currentTimeMillis() - tokenLastUpdateTimeStamp < 10 * 60 * 1000) {
            return tokenCache;
        }
        tokenCache = xxx;
        tokenLastUpdateTimeStamp = System.currentTimeMillis();
        return tokenCache;

but I got a code smell in sonar:

Non-primitive fields should not be "volatile"
Marking an array volatile means that the array itself will always be read fresh and never thread cached, but the items in the array will not be. Similarly, marking a mutable object field volatile means the object reference is volatile but the object itself is not, and other threads may not see updates to the object state.

This can be salvaged with arrays by using the relevant AtomicArray class, such as AtomicIntegerArray, instead. For mutable objects, the volatile should be removed, and some other method should be used to ensure thread-safety, such as synchronization, or ThreadLocal storage.

Noncompliant Code Example
private volatile int [] vInts;  // Noncompliant
private volatile MyObj myObj;  // Noncompliant
Compliant Solution
private AtomicIntegerArray vInts;
private MyObj myObj;

I want to ask: do my usage wrong? I don't think my tokenCache is like myObj;

wangyk
  • 53
  • 7
  • 1
    This looks like micro-optimization. Why not just make getToken() synchronized rather than messing with volatiles? – a guest Dec 30 '20 at 03:17
  • @aguest , thanks, sonar mark my code as a bug. I don't know if the synchronized is better or volatile. I should test it. – wangyk Dec 30 '20 at 03:22
  • 3
    generally speaking, (1) don't optimize until you've shown there's a bottleneck, and (2) it there's some relationship between multiple volatile items, maybe volatile isn't the right tool. – a guest Dec 30 '20 at 03:26
  • 1
    As String is immutable I'm surprised that Sonar flags this as a bug. See https://stackoverflow.com/questions/61628641/is-marking-string-type-reference-as-volatile-safe – tgdavies Dec 30 '20 at 03:35
  • 3
    What version of Sonar are you using? It looks as though this is meant to be fixed: https://jira.sonarsource.com/browse/SONARJAVA-3124 – tgdavies Dec 30 '20 at 03:38
  • @aguest thanks again, I test this, it seems like synchronized is faster than volatile when there is low currency. (2) is a good idea, I got and agree this. – wangyk Dec 30 '20 at 03:43
  • volatile has no real benefits on objects, as it applies only to its reference. I believe this is reason why sonar consider this as codesmell – user902383 Dec 30 '20 at 03:47
  • @tgdavies thanks I think I understand this after view your link, String can mark volatile when the string variable changes. the sonar's version in my company seems like `Data Center EditionVersion 7.9 (build 26994)` under the sonar page – wangyk Dec 30 '20 at 03:56
  • If you are reassigning string object, you could just use `AtomicReference` rather than `volatile`. – Basil Bourque Dec 30 '20 at 04:07
  • @aguest well `volatile` and `synchronized` achieve very different things. you can't suggest to "replace" one with the other – Eugene Dec 31 '20 at 03:39

0 Answers0