0

One should not expose the reference to a non final field if one wants to construct an immutable class - but even for immutable objects like Strings ?

public final class Test { // Test class is meant to be immutable

    private String s; // CAN'T MAKE THIS FINAL

    void onCreate(String s) { // a callback called ONCE after construction
        this.s = new String(s); // do I need to do this ? (protect me from me)
    }

    public String getS() {
        return new String(s); //do I need to do this ?(protect me from the world)
    }
}
Mr_and_Mrs_D
  • 32,208
  • 39
  • 178
  • 361

4 Answers4

2

In theory it is possible through unsafe publication to see an instance of the Test class with an uninitialised (null) s which can also be seen with a correctly initialised s. This is fixable by making s volatile.

However, if you've got some callback happening like that, I think you want to take another look at your design.

If you were to make the class Serializable then you'd have many more problems.

Mr_and_Mrs_D
  • 32,208
  • 39
  • 178
  • 361
Tom Hawtin - tackline
  • 145,806
  • 30
  • 211
  • 305
  • So - read up - unsafe publication essentially means that the object (reference) is made visible to many threads while the object is being constructed - so the threads view the object in an inconsistent state. Should be not an issue if a single thread is constructing and accessing the object - but if the reference is passed to another thread it might see the object in an inconsistent state even after construction ? I don't think so but I have to read up on it – Mr_and_Mrs_D Dec 02 '13 at 16:44
1

I don't think it's necessary. Even in the documentation is said:

Strings are constant; their values cannot be changed after they are created. Because String objects are immutable they can be shared.

So once a String object is created, its value is never changed. If we want to "change" the value of variable a new String object is created. Such as in the toUpperCase method the original string is unchanged, but a new copy is created.

EDIT:

And when considering strings, literals are put into a shared pool, which means that:

String h = "HELLO";
String h1 = "HELLO";

both s1 and s2 refer to the same object.

you can try that following code returns true:

String h = "HELLO";
String h1 = "HELLO";
boolean r = (h==h1);
System.out.println(r);

However you could change the value of the String's value using reflection:

java.lang.reflect.Field valueField = String.class.getDeclaredField("value");
valueField.setAccessible(true);
valueField.set("Original", "Modified".toCharArray());
Kuba Spatny
  • 26,618
  • 9
  • 40
  • 63
  • I was wondering if one could somehow get the reference to the string and do something like `ref="MyString"`. And it is not only for strings – Mr_and_Mrs_D Nov 30 '13 at 15:26
  • Well what would change is that the field would now be referencing to a new object with value "MyString". I think that it should work for all object, if the immutability is done right. Plus the `final` field has nothing to do with the immutability of objects, it wouldn't simply allow to change the reference to which your variable points. – Kuba Spatny Nov 30 '13 at 15:32
  • @Mr_and_Mrs_D well if you want to construct a immutable class, then why not to declare all variables `final`? – Kuba Spatny Nov 30 '13 at 15:44
  • What @KubaSpatny said :) – Vic Nov 30 '13 at 15:45
  • @Vic: have you heard of android ? ;) – Mr_and_Mrs_D Nov 30 '13 at 15:47
  • @Mr_and_Mrs_D, what is special about android in this regard? – Vic Nov 30 '13 at 16:04
  • @Vic: One needs to modify the object after creation (the Context issue) - but this is beside the point anyway - the point is that the field can't be final - or if you wish _if the field can't be final **is defensive copying needed? Is it even enough ?**_ – Mr_and_Mrs_D Nov 30 '13 at 16:09
  • @Mr_and_Mrs_D your problem is that you have a setter for your field, so it can be changed at any point. As such, defensive copying is irrelevant. –  Nov 30 '13 at 16:28
  • @jgm: where is the setter ? the onCreate is meant to be a callback called once - not a getter - think of it as package private – Mr_and_Mrs_D Nov 30 '13 at 16:38
  • @Mr_and_Mrs_D it's a public method which changes the field: it's a setter. Something like Vic's answer will help you (although you should synchronise the method to be safe). But if we assume that the field is immutable for argument's sake then no, you don't need to return a defensive copy of an immutable field. Bear in mind that if you have any methods which return collections then both the collections and their contents should be either immutable or copies. –  Nov 30 '13 at 16:48
  • @jgm: made it package private :) - my issue is with defensive copying and mostly with the getter - added this "onCreate" for say completeness – Mr_and_Mrs_D Nov 30 '13 at 16:51
1

Technically, if you really want a immutable class in Java you have to make sure that an instance of your class can not be changed after it's created. Therefore all its fields can be final and if they are "exposed" to the world via getters, for example, those fields must either be immutable themselves (as strings are) or not being returned to the outer world (kept private and creating defensive copies of them in getters), so the original field value stays the same. This immutability must not be prone to being broken by inheriting from this class as well.

You can read more about it in Effective Java - a book by Joshua Bloch, or take some notes from the internet, like from here.

Regarding your recent update to the post, here's a suggestion that ensures the initialization was only made once:

private String s; // CAN'T MAKE THIS FINAL
private boolean stringWasSet = false;

public void onCreate(String s) { // a callback called ONCE after construction
    if (!stringWasSet) {
        this.s = s; // No need for defensive copy here, if the variable itself is immutable, like String
        stringWasSet = true;
    }
}

public String getS() {
    return s; // No need for defensive copy here, if the variable itself is immutable, like String
}
Vic
  • 21,473
  • 11
  • 76
  • 97
  • I've read effective Java - my question is how we create an immutable class with non final but immutable fields - it is not an easy one – Mr_and_Mrs_D Nov 30 '13 at 16:00
  • @Mr_and_Mrs_D well if the field contains immutable object then you can return it (because its immutability should ensure that it wont be changed) but for your class, you can't have any methods that would allow changing the non-final field – Kuba Spatny Nov 30 '13 at 16:02
  • @Mr_and_Mrs_D, are those fields not private/have setters? – Vic Nov 30 '13 at 16:03
  • @Vic: of course not - I want to create an immutable class - so why should I add setters ?? What worries me is attacks I haven't thought of - it happens every other day in security :) So do I need defensive copying ? Or is there some other way (apart from reflection of course) ? – Mr_and_Mrs_D Nov 30 '13 at 16:06
  • So your class is immutable; you don't HAVE to define the fields final, but it doesn't hurt and declares your intentions to the next developer who looks through your code. – Vic Nov 30 '13 at 16:11
1

It does not matter whether this class is immutable (for any definition of immutable). In particular, it does not matter if the reference s is ever changed to point to a different string. The string object is immutable, so you don't need to copy it. Without defensive copying, callers of getS will get references to the same string object used by Test's methods and by other callers of getS. That does not matter because nothing1 they do to this string will affect other referents. It'd be a waste of time and memory.

1 I'm ignoring reflection. Code that maliciously uses reflection like this can break almost anything, and is not written by accident or hard to spot. It is not even remotely practical to worry about this case.