Suggest using AtomicReference to fix java:S2696

[trajano (Archimedes Trajano)]

Presently java:S2696 says why it’s an issue.

It would be nice for it to provide another solution especially if the code that is overwriting the field is an overridden method. One approach would be to use AtomicReference

Here’s an example of code that would be flagged

@Configuration
@RequiredArgsConstructor
public class HibernateStorageConfiguration implements InitializingBean {
  private static final RedisConnectionFactory configuredRedisConnectionFactory;
  private final RedisConnectionFactory redisConnectionFactory;
  public static RedisConnectionFactory getConfiguredRedisConnectionFactory() {
    return configuredRedisConnectionFactory;
  }
  @Override
  public void afterPropertiesSet() {
    configuredRedisConnectionFactory = redisConnectionFactory;
  }
}

Here’s the fix

@Configuration
@RequiredArgsConstructor
public class HibernateStorageConfiguration implements InitializingBean {

  private static final AtomicReference<RedisConnectionFactory> configuredRedisConnectionFactoryRef =
      new AtomicReference<>();

  private final RedisConnectionFactory redisConnectionFactory;

  public static RedisConnectionFactory getConfiguredRedisConnectionFactory() {

    return configuredRedisConnectionFactoryRef.get();
  }

  @Override
  public void afterPropertiesSet() {

    configuredRedisConnectionFactoryRef.set(redisConnectionFactory);
  }

}

The text could be worded as

Correctly updating a static field from a non-static method is tricky to get right and could easily lead to bugs if there are multiple class instances and/or multiple threads in play. Ideally, static fields are only updated from synchronized static methods.

Another approach would be to replace the static field to use a static final AtomicReference, AtomicBoolean, AtomicInteger, AtomicILong, or AtomicIDouble which guarantees the value/reference is updated atomically.

Compiant code examples

public class MyClass {
  private static int count = 0;
  public syntchronized static void doSomething() { // Compliant
    count++;  
  }
}

public class MyClass {
  private static final AtomicInteger countRef = new AtomicInteger(0);
  public void doSomething() { 
    countRef.incrementAndGet();   // Compliant
  }
}

[Dorian_Burihabwa]

Hey @trajano,

Thank you for working out a concise and clear suggestion to improve this rule description
It is indeed a bit frustrating that the current rule description does not offer a compliant alternative to fix the issue.

I like your suggestion but I feel like it only solves part of the problem: We are now free of runtime errors due to concurrent modifications but it is not really easier to understand what the value of the static field should be at any given time (without looking at the whole flow of the program).

Ideally, the rule would nudge developers into doing one of these three things (in order of preference):

1. Remove modifications from instance members on the static field
2. Change the static field into an instance field
3. Wrap the static field into a compatible Atomic type

Does that order seem reasonable to you?

Cheers,

Dorian