What is wrong in sharing Mutable State?

Question

In Java Concurrency in Practice chapter # 3 author has suggested not to share the mutable state. Further he has added that below code is not a good way to share the states.

class UnsafeStates {

    private String[] states = new String[] {
      "AK", "AL"
    };

    public String[] getStates() {
        return states;
    }
}

From the book:

Publishing states in this way is problematic because any caller can modify its contents. In this case, the states array has escaped its intended scope, because what was supposed to be private state has been effectively made public.

My question here is: we often use getter and setters to access the class level private mutable variables. if it is not the correct way, what is the correct way to share the state? what is the proper way to encapsulate states ?

Getters and setters are good for *primitives*. In this case you are getting an *array*, so you can change its content without a getter or a setter. — RealSkeptic, Nov 06 '19 at 12:11
Using only the getter still allows a consumer to modify the array. Perhaps the author is suggesting that in this case you return a copy/clone of the array. The discussion gets even more in depth when exposing complex objects, often referring back to the Law Of Demeter and suggesting that objects should explicitly expose their complete set of operations, as opposed to exposing internal objects (like in your example) which themselves have operations that modify their state. — David, Nov 06 '19 at 12:16

Sweeper · Accepted Answer · 2019-11-06T13:29:22.360

For primitive types, int, float etc, using a simple getter like this does not allow the caller to set its value:

someObj.getSomeInt() = 10; // error!

However, with an array, you could change its contents from the outside, which might be undesirable depending on the situation:

someObj.getSomeArray()[0] = newValue; // perfectly fine

This could lead to problems where a field is unexpectedly changed by other parts of code, causing hard-to-track bugs.

What you can do instead, is to return a copy of the array:

public String[] getStates() {
    return Arrays.copyOf(states, states.length);
}

This way, even the caller changes the contents of the returned array, the array held by the object won't be affected.

score 0 · Answer 2 · answered Nov 06 '19 at 12:20

With what you have it is possible for someone to change the content of your private array just through the getter itself:

public static void main(String[] args) {
  UnsafeStates us = new UnsafeStates();
  us.getStates()[0] = "VT";

  System.out.println(Arrays.toString(us.getStates());
}

Output:

[VT, AR]

If you want to encapsulate your States and make it so they cannot change then it might be better to make an enum:

public enum SafeStates {
  AR,
  AL
}

Creating an enum gives a couple advantages. It allows exact vales that people can use. They can't be modified, its easy to test against and can easily do a switch statement on it. The only downfall for going with an enum is that the values have to be known ahead of time. I.E you code for it. Cannot be created at run time.

score 0 · Answer 3 · answered Nov 06 '19 at 13:49

This question seems to be asked with respect to concurrency in particular.

Firstly, of course, there is the possibility of modifying non-primitive objects obtained via simple-minded getters; as others have pointed out, this is a risk even with single-threaded programs. The way to avoid this is to return a copy of an array, or an unmodifiable instance of a collection: see for example Collections.unmodifiableList.

However, for programs using concurrency, there is risk of returning the actual object (i.e., not a copy) even if the caller of the getter does not attempt to modify the returned object. Because of concurrent execution, the object could change "while he is looking at it", and in general this lack of synchronization could cause the program to malfunction.

It's difficult to turn the original getStates example into a convincing illustration of my point, but imagine a getter that returns a Map instead. Inside the owning object, correct synchronization may be implemented. However, a getTheMap method that returns just a reference to the Map is an invitation for the caller to call Map methods (even if just map.get) without synchronization.

There are basically two options to avoid the problem: (1) return a deep copy; an unmodifiable wrapper will not suffice in this case, and it should be a deep copy otherwise we just have the same problem one layer down, or (2) do not return unmediated references; instead, extend the method repertoire to provide exactly what is supportable, with correct internal synchronization.

What is wrong in sharing Mutable State?

3 Answers3