How to Make a Defensive Copy of a Thread-Safe Collection

Question

I have the following class with a private thread safe collection declared as such:

final private ConcurrentHashMap<Book,BookLog> booklogMap;

According to this page, because the date is mutable and it's being used in an immutable class, a defensive copy must be made to avoid changing the object after creation. They make a defensive copy in the constructor like this:

fDateOfDiscovery = new Date(aDateOfDiscovery.getTime())

and in the getter like this:

 public Date getDateOfDiscovery() {
    return new Date(fDateOfDiscovery.getTime());
  }

How then would I properly create a defensive copy of the ConcurrentHashMap in my constructor? I can't use Collections.modifiableMap(), because it will run into a cast issue.

`new ConcurrentHashMap(booklogMap)`? Do you need to make a copy of the contents as well? — resueman, Feb 11 '16 at 21:12
Why will using `Collections.unmodifiableMap()` run into a cast issue? — Tunaki, Feb 11 '16 at 21:14
@Tunaki - When I tired it, it said to add a cast of ConcurrentHashMap to the Collections.unmodifiableMap(). — AnnabelleRosemond, Feb 11 '16 at 22:21

score 1 · Answer 1 · answered Feb 11 '16 at 21:20

You may or may not want to use Collections.unmodifiableMap() since that returns a Map, not a ConcurrentHashMap, and you probably can't cast it (though I haven't tried). That may not be such a bad thing as other classes may not need to know that this particular Map is of the Concurrent Hash variety.

@resueman suggests in comments that you could use

new ConcurrentHashMap<Book, Booklog>(booklogMap);

That creates a (defensive) copy of the ConcurrentHashMap itself, but does not create copies of the contents of the Map. That may be ok, or it may not, depending on what the users of the Map want to do with it.

And, if you decide you need to copy the contents, you have to decide whether to copy the field values that those copies contain. And so on, until you stop.

My issue was, I've been using the method as you and reueman suggested. However, it doesn't make a copy. The user will be able to add a book and any log that may go with it, so it's going to change from time to time. How would I make a copy of everything(field values)? — AnnabelleRosemond, Feb 11 '16 at 22:23

score 0 · Answer 2 · edited May 23 '17 at 10:28

Edit with example

this will copy the input map, safely provide an immutable copy of the map when request, and safely return an element of the map when needed:

import java.util.Collections;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;


public class CopyMap {

    //just an example Long and String are both immutable
    //replace Long for Book (Book has to implement hashcode and equals.
    // replace String for Booklog
    private final Map<Long, String> map;

    public CopyMap(Map<Long,String> map) {
        this.map = new ConcurrentHashMap<>(map);
    }

    // get a non editable copy of the whole map
    // threadsafe because this.map is concurrentHashMap
    // no synchronization needed
    public Map<Long,String> getMap() {
        return Collections.unmodifiableMap(this.map);
    }

    //get only one of the elements
    //thread safe because implementation is ConcurrentHashMap
    //no synchronization needed
    public String get(Long key) {
        return map.get(key);
    }

    //just to try it, with -ea enabled
    public static void main(String... args) {
        final Map<Long,String> map1 = new ConcurrentHashMap<>();
        map1.put(1l,"1");
        map1.put(2l,"2");
        map1.put(3l,"3");

        final CopyMap copyMap = new CopyMap(map1);

        assert(copyMap.getMap().equals(map1));
        assert(copyMap.getMap()!=map1);

        assert(copyMap.get(1l).equals("1"));
        assert(copyMap.get(2l).equals("2"));
        assert(copyMap.get(3l).equals("3"));
    }
}

Original for historical purposes: A few things,

Firstly, it is better if you define your variables/fields/constants all as interfaces as opposed to specific concrete classes. So, you should define you map as:

//conventions usually prefer private final, as opposed to final private
private final Map<Book,BookLog> booklogMap;

Then, and Secondly, you can indeed use the Collections.unmodifiableMap()

So an example would be:

//package and imports here
public class MyClass {

  private final Map<Book,BookLog> booklogMap;

  public MyClass(Map<Book, BookLog> booklogMap) {
    this.booklogMap = Collections.unmodifiableMap(booklogMap);
  }
}

Thirdly, for you to have true immutability, you need to make the whole hierarchy of objects immutable. So your classes Book and BookLog need to be immutable as well.

Otherwise, you need to deep copy all the Book(s), and BookLog(s) one by one in the constructor.

Thanks for this, but a few issues, I use a concurrenthashmap for a reason, and Book/BookLog is in fact immutable — AnnabelleRosemond, Feb 11 '16 at 22:20
Please see my edit: I created that little example. I hope it helps. It using Long and String because those 2 are immutable (replace them with your classes). — portenez, Feb 12 '16 at 01:19
BTW the defensive copy is usually only required in the getters/setters, so that clients don't change the collection your object owns. However, if that's what you want (protecting the owned collection from external mutations) the ```Collections.unmodifiable()``` will protect your collection the same way. — portenez, Feb 12 '16 at 01:58

score 0 · Answer 3 · answered Feb 12 '16 at 01:40

You have said in the comments that Book and BookLog are immutable, so you don't need to make defensive copies of them. But you also say in the comments that new ConcurrentHashMap<Book, Booklog>(booklogMap); doesn't make a copy, which doesn't make any sense, because it does:

public static void main(String[] args) {
    Map<String, Integer> map = new ConcurrentHashMap<>();
    map.put("apples", 3);
    System.out.println(map);
    // prints {apples=3}
    Map<String, Integer> map2 = new ConcurrentHashMap<>(map);
    map2.put("oranges", 1);
    System.out.println(map2);
    // prints {oranges=1, apples=3}
    System.out.println(map);
    //prints {apples=3}
}

If these answers aren't helping you, then you really need to clarify your question. Which of your objects are immutable? What mutation are you trying to prevent? Can you give an example?

In another post, several answers suggested it doesn't make a copy. Sorry about that. The mutations I'm trying to prevent are users adding to the collection after the bookLogMap as been created in the constructor. Exactly how a user can modify the date in the page I linked too. I don't want that. — AnnabelleRosemond, Feb 12 '16 at 12:41
@AnnabelleRosemond, do you mean you have a `getBookMap` method that exposes the map? in that case, you could delete the method, or make a defensive copy in the getter now that you now it works. — DavidS, Feb 12 '16 at 16:01

How to Make a Defensive Copy of a Thread-Safe Collection

3 Answers3