subtle concurrency bug: when file pointer calls update() outside lock its myNode can point to already obsolete node with null myFileAndUrl (can happen during intensive modifications)

2026-09-27 10:03:11 +07:00 · 2017-01-17 15:10:16 +03:00
parent 1e6ab8fb7b
commit 765b9f4aac
4 changed files with 94 additions and 20 deletions
@@ -44,7 +44,7 @@ class FilePointerPartNode {

  // in case there is file pointer exists for this part, its info is saved here
  volatile Pair<VirtualFile, String> myFileAndUrl; // must not be both null
-  private volatile long myLastUpdated = -1;
+  volatile long myLastUpdated = -1;
  volatile int useCount;

  int pointersUnder;   // number of alive pointers in this node plus all nodes beneath
@@ -171,7 +171,7 @@ class FilePointerPartNode {
    if (index == path.length() // query matched entirely
      && index - start == part.length()) {
      if (leaves == null) {
-        pointersUnder+=pointersToStore; // the pointer is going to be written here
+        pointersUnder += pointersToStore; // the pointer is going to be written here
      }
      return this;
    }
@@ -183,7 +183,7 @@ class FilePointerPartNode {
        if (i != index && (i > index+1 || path.charAt(index) != '/' || index == 0)) {
          FilePointerPartNode node = child.findPointerOrCreate(path, index, fileAndUrl, pointersToStore);
          if (node.leaves == null) {
-            pointersUnder+=pointersToStore; // the new node's been created
+            pointersUnder += pointersToStore; // the new node's been created
          }
          return node;
        }
@@ -191,9 +191,9 @@ class FilePointerPartNode {
      // cannot insert to children, create child node manually
      String pathRest = path.substring(index);
      FilePointerPartNode newNode = new FilePointerPartNode(pathRest, this, fileAndUrl);
-      newNode.pointersUnder+=pointersToStore;
+      newNode.pointersUnder += pointersToStore;
      children = ArrayUtil.append(children, newNode);
-      pointersUnder+=pointersToStore;
+      pointersUnder += pointersToStore;
      return newNode;
    }
    // else there is no match
@@ -214,11 +214,11 @@ class FilePointerPartNode {
    splittedAway.pointersUnder = pointersUnder;
    splittedAway.useCount = useCount;
    splittedAway.associate(leaves, myFileAndUrl);
-    associate(null, null);
    useCount = 0;
    part = commonPredecessor;
    children = newNode == this ? new FilePointerPartNode[]{splittedAway} : new FilePointerPartNode[]{splittedAway, newNode};
    pointersUnder+=pointersToStore;
+    associate(null, null);
    return newNode;
  }

@@ -248,11 +248,12 @@ class FilePointerPartNode {
    return indexOfFirstDifferentChar(string, string.length() - end.length(), end, 0) == string.length();
  }

-  @NotNull
+  @Nullable("null means this node's myFileAndUrl became invalid (e.g. after splitting into two other nodes)")
  // returns pair.second != null always
  Pair<VirtualFile, String> update() {
    long lastUpdated = myLastUpdated;
    Pair<VirtualFile, String> fileAndUrl = myFileAndUrl;
+    if (fileAndUrl == null) return null;
    long fsModCount = ourManagingFS.getStructureModificationCount();
    if (lastUpdated == fsModCount) return fileAndUrl;
    VirtualFile file = fileAndUrl.first;
@@ -294,7 +295,7 @@ class FilePointerPartNode {
    else {
      result = fileAndUrl;
    }
-    myLastUpdated = fsModCount;
+    myLastUpdated = fsModCount; // must be the last
    return result;
  }

@@ -315,6 +316,9 @@ class FilePointerPartNode {
  }

  void associate(Object leaves, Pair<VirtualFile, String> fileAndUrl) {
+    this.leaves = leaves;
+    myFileAndUrl = fileAndUrl;
+    // assign myNode last because .update() reads that field outside lock
    if (leaves != null) {
      if (leaves instanceof VirtualFilePointerImpl) {
        ((VirtualFilePointerImpl)leaves).myNode = this;
@@ -325,8 +329,6 @@ class FilePointerPartNode {
        }
      }
    }
-    this.leaves = leaves;
-    myFileAndUrl = fileAndUrl;
    myLastUpdated = -1;
  }

@@ -45,7 +45,7 @@ class VirtualFilePointerImpl extends TraceableDisposable implements VirtualFileP
  @NotNull
  public String getFileName() {
    if (!checkDisposed()) return "";
-    Pair<VirtualFile, String> result = myNode.update();
+    Pair<VirtualFile, String> result = update();
    VirtualFile file = result.first;
    if (file != null) {
      return file.getName();
@@ -55,10 +55,21 @@ class VirtualFilePointerImpl extends TraceableDisposable implements VirtualFileP
    return index >= 0 ? url.substring(index + 1) : url;
  }

+  @NotNull
+  private Pair<VirtualFile, String> update() {
+    while (true) {
+      Pair<VirtualFile, String> result = myNode.update();
+      if (result != null) {
+        return result;
+      }
+      // otherwise the node is becoming invalid, retry
+    }
+  }
+
  @Override
  public VirtualFile getFile() {
    if (!checkDisposed()) return null;
-    Pair<VirtualFile, String> result = myNode.update();
+    Pair<VirtualFile, String> result = update();
    return result.first;
  }

@@ -66,15 +77,10 @@ class VirtualFilePointerImpl extends TraceableDisposable implements VirtualFileP
  @NotNull
  public String getUrl() {
    if (isDisposed()) return "";
-    Pair<VirtualFile, String> update = myNode.update();
+    Pair<VirtualFile, String> update = update();
    return update.second;
  }

-  @NotNull
-  private String getUrlNoUpdate() {
-    return isDisposed() ? "" : myNode.myFileAndUrl.second;
-  }
-
  @Override
  @NotNull
  public String getPresentableUrl() {
@@ -94,13 +100,13 @@ class VirtualFilePointerImpl extends TraceableDisposable implements VirtualFileP

  @Override
  public boolean isValid() {
-    Pair<VirtualFile, String> result = isDisposed() ? null : myNode.update();
+    Pair<VirtualFile, String> result = isDisposed() ? null : update();
    return result != null && result.first != null;
  }

  @Override
  public String toString() {
-    return getUrlNoUpdate();
+    return isDisposed() ? "" : myNode.myFileAndUrl.second;
  }

  public void dispose() {
@@ -489,6 +489,7 @@ public class VirtualFilePointerManagerImpl extends VirtualFilePointerManager imp
      synchronized (this) {
        String urlBefore = node.myFileAndUrl.second;
        Pair<VirtualFile,String> after = node.update();
+        assert after != null : "can't invalidate inside modification";
        String urlAfter = after.second;
        if (URL_COMPARATOR.compare(urlBefore, urlAfter) != 0 || !urlAfter.endsWith(node.part)) {
          List<VirtualFilePointerImpl> myPointers = new SmartList<>();
@@ -53,9 +53,12 @@ import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
+import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;

 /**
 *  @author dsl
@@ -872,4 +875,66 @@ public class VirtualFilePointerTest extends PlatformTestCase {

    assertEquals(0, ((VirtualFilePointerManagerImpl)VirtualFilePointerManager.getInstance()).numberOfCachedUrlToIdentity());
  }
+
+  private volatile boolean run;
+  private volatile Throwable exception;
+  public void testStressConcurrentAccess() throws Throwable {
+    final File tempDirectory = createTempDirectory();
+    VirtualFilePointer fileToCreatePointer = createPointerByFile(tempDirectory, null);
+    assertNotNull(fileToCreatePointer);
+
+    VirtualFilePointerListener listener = new VirtualFilePointerListener() {
+      @Override
+      public void beforeValidityChanged(@NotNull VirtualFilePointer[] pointers) {
+
+      }
+
+      @Override
+      public void validityChanged(@NotNull VirtualFilePointer[] pointers) {
+
+      }
+    };
+    for (int i=0; i<10_000;i++) {
+      Disposable disposable = Disposer.newDisposable();
+      // supply listener to separate pointers under one root so that it will be removed on dispose
+      VirtualFilePointerImpl bb =
+        (VirtualFilePointerImpl)VirtualFilePointerManager.getInstance().create(fileToCreatePointer.getUrl() + "/bb", disposable, listener);
+
+      if (i%1000==0)System.out.println("i = " + i);
+
+      int N = Runtime.getRuntime().availableProcessors();
+      CountDownLatch ready = new CountDownLatch(N);
+      Runnable read = () -> {
+        try {
+          ready.countDown();
+          while (run) {
+            bb.myNode.myLastUpdated = -15;
+            bb.getUrl();
+          }
+        }
+        catch (Throwable e) {
+          exception = e;
+        }
+      };
+
+      run = true;
+      List<Thread> threads = IntStream.range(0, N).mapToObj(n -> new Thread(read, "reader"+n)).collect(Collectors.toList());
+      threads.forEach(Thread::start);
+      ready.await();
+
+      VirtualFilePointer bc = VirtualFilePointerManager.getInstance().create(fileToCreatePointer.getUrl() + "/b/c", disposable, listener);
+
+      run = false;
+      threads.forEach(thread -> {
+        try {
+          thread.join();
+        }
+        catch (InterruptedException e) {
+          throw new RuntimeException(e);
+        }
+      });
+      if (exception !=null) throw exception;
+      Disposer.dispose(disposable);
+    }
+  }
 }