Bucket: fix concurrent Modification during save() - fixes #1721

Take a snapshot of the property map before processing the properties

#1721

Author: Jörg Kubitz

resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/localstore/Bucket.java

@@ -30,6 +30,8 @@
 import java.util.Iterator;
 import java.util.Map;
 import java.util.WeakHashMap;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
 import org.eclipse.core.internal.resources.ResourceException;
 import org.eclipse.core.internal.resources.ResourceStatus;
 import org.eclipse.core.internal.utils.Messages;
@@ -169,28 +171,28 @@ public void beforeSaving(Bucket bucket) throws CoreException {
 	 * where the key is the path of the object we are storing history for, and
 	 * the value is the history entry data (UUID,timestamp) pairs.
 	 */
-	private final Map<String, Object> entries;
-	private SoftReference<Map<Object, Map<String, Object>>> entriesCache;
+	private final ConcurrentMap<String, Object> entries;
+	private volatile SoftReference<Map<String, Map<String, Object>>> entriesCache;
 
 	/**
 	 * The file system location of this bucket index file.
 	 */
-	private File location;
+	private volatile File location;
 	/**
 	 * Whether the in-memory bucket is dirty and needs saving
 	 */
-	private boolean needSaving = false;
+	private volatile boolean needSaving = false;
 	/**
 	 * The project name for the bucket currently loaded. <code>null</code> if this is the root bucket.
 	 */
-	protected String projectName;
+	protected volatile String projectName;
 
 	public Bucket() {
 		this(false);
 	}
 
 	public Bucket(boolean cacheEntries) {
-		this.entries = new HashMap<>();
+		this.entries = new ConcurrentHashMap<>();
 		if (cacheEntries) {
 			entriesCache = new SoftReference<>(null);
 		}
@@ -332,7 +334,7 @@ public void load(String newProjectName, File baseLocation, boolean force) throws
 				loadedEntries = loadEntries(this.location);
 			} else {
 				if (isCachingEnabled()) {
-					Map<Object, Map<String, Object>> cache = entriesCache.get();
+					Map<String, Map<String, Object>> cache = entriesCache.get();
 					if (cache != null) {
 						loadedEntries = cache.get(createBucketKey());
 					}
@@ -355,7 +357,7 @@ boolean isCachingEnabled() {
 		return entriesCache != null;
 	}
 
-	private Object createBucketKey() {
+	private String createBucketKey() {
 		return this.location == null ? null : this.location.getAbsolutePath();
 	}
 
@@ -396,25 +398,24 @@ private String readEntryKey(DataInputStream source) throws IOException {
 	 * Saves this bucket's contents back to its location.
 	 */
 	public void save() throws CoreException {
+		// We do need to make a copy of this.entries for caching, because that instance is reused
+		// And we need atomic copy to prevent concurrent modification during save();
+		Map<String, Object> entriesSnapshot = Map.copyOf(this.entries);
 		if (isCachingEnabled()) {
-			Object key = createBucketKey();
+			String key = createBucketKey();
 			if (key != null) {
-				// we do need to make a copy from this.entries because that instance is reused
-				@SuppressWarnings("unchecked")
-				java.util.Map.Entry<String, Object>[] a = new java.util.Map.Entry[0];
-				java.util.Map<String, Object> denseCopy = java.util.Map.ofEntries(this.entries.entrySet().toArray(a));
-				Map<Object, Map<String, Object>> cache = entriesCache.get();
+				Map<String, Map<String, Object>> cache = entriesCache.get();
 				if (cache == null) {
 					cache = new WeakHashMap<>();
 					entriesCache = new SoftReference<>(cache);
 				}
-				cache.put(key, denseCopy); // remember the entries in cache
+				cache.put(key, entriesSnapshot); // remember the entries in cache
 			}
 		}
 		if (!needSaving)
 			return;
 		try {
-			if (entries.isEmpty()) {
+			if (entriesSnapshot.isEmpty()) {
 				needSaving = false;
 				cleanUp(location);
 				return;
@@ -426,8 +427,8 @@ public void save() throws CoreException {
 			parent.mkdirs();
 			try (DataOutputStream destination = new DataOutputStream(new BufferedOutputStream(new FileOutputStream(location), 8192))) {
 				destination.write(getVersion());
-				destination.writeInt(entries.size());
-				for (java.util.Map.Entry<String, Object> entry : entries.entrySet()) {
+				destination.writeInt(entriesSnapshot.size());
+				for (java.util.Map.Entry<String, Object> entry : entriesSnapshot.entrySet()) {
 					writeEntryKey(destination, entry.getKey());
 					writeEntryValue(destination, entry.getValue());
 				}