IJPL-179451: ChangeTrackingValueContainer.addToRemoved hangs

GitOrigin-RevId: 872950de4070d1c601857ff5bdc3ea81c9b86a19

platform/util/src/com/intellij/util/indexing/impl/ChangeTrackingValueContainer.java

@@ -7,12 +7,13 @@ import com.intellij.util.indexing.ValueContainer;
 import com.intellij.util.io.DataExternalizer;
 import com.intellij.util.io.DataInputOutputUtil;
 import it.unimi.dsi.fastutil.ints.IntOpenHashSet;
-import it.unimi.dsi.fastutil.ints.IntSet;
 import org.jetbrains.annotations.ApiStatus.Internal;
 import org.jetbrains.annotations.NotNull;
 
 import java.io.DataOutput;
 import java.io.IOException;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import java.util.function.IntConsumer;
 
 /**
  * Container balances between keeping the changes as changes, and merging (applying) them.
@@ -24,9 +25,67 @@ import java.io.IOException;
  */
 @Internal
 public class ChangeTrackingValueContainer<Value> extends UpdatableValueContainer<Value> {
+
+  protected static class ThreadSafeIntSet {
+    private final IntOpenHashSet delegate = new IntOpenHashSet(1);
+    private final ReentrantReadWriteLock rwLock = new ReentrantReadWriteLock();
+
+    private <T> T withWriteLock(Computable<T> block) {
+      rwLock.writeLock().lock();
+      try {
+        return block.compute();
+      }
+      finally {
+        rwLock.writeLock().unlock();
+      }
+    }
+
+    private <T> T withReadLock(Computable<T> block) {
+      rwLock.readLock().lock();
+      try {
+        return block.compute();
+      }
+      finally {
+        rwLock.readLock().unlock();
+      }
+    }
+
+    boolean add(int id) {
+      return withWriteLock(() -> delegate.add(id));
+    }
+
+    void forEach(IntConsumer intConsumer) {
+      withReadLock(() -> {
+        delegate.forEach(intConsumer);
+        return null;
+      });
+    }
+
+    boolean isEmpty() {
+      return withReadLock(() -> delegate.isEmpty());
+    }
+
+    int[] toIntArray() {
+      return withReadLock(() -> delegate.toIntArray());
+    }
+
+    public void remove(int id) {
+      withWriteLock(() -> {
+        delegate.remove(id);
+        return null;
+      });
+    }
+  }
+
   // there is no volatile as we modify under write lock and read under read lock
+  // TODO RC: myInvalidated can be updated under read lock (through FileBasedIndexImpl.ensureUpToDate),
+  //          which technically means that we modify set under read lock.
+  //          We have a lot of reports that IDE freezes in IntOpenHashSet.add, and it seems that we indeed never exit that method.
+  //          Inside IntOpenHashSet.add there is a while loop. If it never exits, then we either have a problem with internal
+  //          structure consistency, or we have a bug in IntOpenHashSet implementation. Former is more realistic, let's check
+  //          if this is true by making myInvalidated thread-safe.
   protected ValueContainerImpl<Value> myAdded;
-  protected IntSet myInvalidated;
+  protected ThreadSafeIntSet myInvalidated;
 
 
   //TODO RC: volatile field(s) here seems suspicious/ambiguous to me.
@@ -73,7 +132,7 @@ public class ChangeTrackingValueContainer<Value> extends UpdatableValueContainer
     boolean wasRemovedFromAdded = removeFromAdded(inputId);
     //RC: It is teasing to short-circuit here if wasRemovedFromAdded=true -- seems like no need to add inputId to invalidatedIds?
     //    Wrong: inputId could be contained in the (not yet loaded) <mergedSnapshot> -- inputId still needs to be in invalidatedIds then.
-    //    I.e consider scenario:
+    //    I.e. consider scenario:
     //    1) container X created: { merged=null, added=[], invalidated=[] }
     //       underlying container (to-be-mergedSnapshot, not yet loaded) = [..., (inputId, value), ... ]
     //    2) X.addValue(inputId, value) => X{ merged=null, added=[(inputId, value)], invalidated=[] }
@@ -88,7 +147,7 @@ public class ChangeTrackingValueContainer<Value> extends UpdatableValueContainer
 
   private boolean addToRemoved(int inputId) {
     if (myInvalidated == null) {
-      myInvalidated = new IntOpenHashSet(1);
+      myInvalidated = new ThreadSafeIntSet();
     }
     return myInvalidated.add(inputId);
   }
@@ -196,9 +255,9 @@ public class ChangeTrackingValueContainer<Value> extends UpdatableValueContainer
 
   public void saveDiffTo(@NotNull DataOutput out,
                          @NotNull DataExternalizer<? super Value> externalizer) throws IOException {
-    IntSet set = myInvalidated;
+    ThreadSafeIntSet set = myInvalidated;
     if (set != null && !set.isEmpty()) {
-      for (int inputId : myInvalidated.toIntArray()) {
+      for (int inputId : set.toIntArray()) {
         DataInputOutputUtil.writeINT(out, -inputId); // mark inputId as invalid, to be processed on load in ValueContainerImpl.readFrom
       }
     }