From 765b9f4aacdf88dbfbd3cb0e9ce9eaa64f30c339 Mon Sep 17 00:00:00 2001 From: Alexey Kudravtsev Date: Tue, 17 Jan 2017 15:06:23 +0300 Subject: [PATCH] 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) --- .../openapi/vfs/impl/FilePointerPartNode.java | 22 ++++--- .../vfs/impl/VirtualFilePointerImpl.java | 26 +++++--- .../impl/VirtualFilePointerManagerImpl.java | 1 + .../vfs/impl/VirtualFilePointerTest.java | 65 +++++++++++++++++++ 4 files changed, 94 insertions(+), 20 deletions(-) diff --git a/platform/platform-impl/src/com/intellij/openapi/vfs/impl/FilePointerPartNode.java b/platform/platform-impl/src/com/intellij/openapi/vfs/impl/FilePointerPartNode.java index da83b63f407d..274234bc8ec7 100644 --- a/platform/platform-impl/src/com/intellij/openapi/vfs/impl/FilePointerPartNode.java +++ b/platform/platform-impl/src/com/intellij/openapi/vfs/impl/FilePointerPartNode.java @@ -44,7 +44,7 @@ class FilePointerPartNode { // in case there is file pointer exists for this part, its info is saved here volatile Pair 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 update() { long lastUpdated = myLastUpdated; Pair 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 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; } diff --git a/platform/platform-impl/src/com/intellij/openapi/vfs/impl/VirtualFilePointerImpl.java b/platform/platform-impl/src/com/intellij/openapi/vfs/impl/VirtualFilePointerImpl.java index dea9432d0064..bc07a2bd2742 100644 --- a/platform/platform-impl/src/com/intellij/openapi/vfs/impl/VirtualFilePointerImpl.java +++ b/platform/platform-impl/src/com/intellij/openapi/vfs/impl/VirtualFilePointerImpl.java @@ -45,7 +45,7 @@ class VirtualFilePointerImpl extends TraceableDisposable implements VirtualFileP @NotNull public String getFileName() { if (!checkDisposed()) return ""; - Pair result = myNode.update(); + Pair 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 update() { + while (true) { + Pair result = myNode.update(); + if (result != null) { + return result; + } + // otherwise the node is becoming invalid, retry + } + } + @Override public VirtualFile getFile() { if (!checkDisposed()) return null; - Pair result = myNode.update(); + Pair result = update(); return result.first; } @@ -66,15 +77,10 @@ class VirtualFilePointerImpl extends TraceableDisposable implements VirtualFileP @NotNull public String getUrl() { if (isDisposed()) return ""; - Pair update = myNode.update(); + Pair 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 result = isDisposed() ? null : myNode.update(); + Pair result = isDisposed() ? null : update(); return result != null && result.first != null; } @Override public String toString() { - return getUrlNoUpdate(); + return isDisposed() ? "" : myNode.myFileAndUrl.second; } public void dispose() { diff --git a/platform/platform-impl/src/com/intellij/openapi/vfs/impl/VirtualFilePointerManagerImpl.java b/platform/platform-impl/src/com/intellij/openapi/vfs/impl/VirtualFilePointerManagerImpl.java index 46afbc183497..eeabd8b68c52 100644 --- a/platform/platform-impl/src/com/intellij/openapi/vfs/impl/VirtualFilePointerManagerImpl.java +++ b/platform/platform-impl/src/com/intellij/openapi/vfs/impl/VirtualFilePointerManagerImpl.java @@ -489,6 +489,7 @@ public class VirtualFilePointerManagerImpl extends VirtualFilePointerManager imp synchronized (this) { String urlBefore = node.myFileAndUrl.second; Pair 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 myPointers = new SmartList<>(); diff --git a/platform/platform-tests/testSrc/com/intellij/openapi/vfs/impl/VirtualFilePointerTest.java b/platform/platform-tests/testSrc/com/intellij/openapi/vfs/impl/VirtualFilePointerTest.java index afd2a6bb7d3a..323512954f6a 100644 --- a/platform/platform-tests/testSrc/com/intellij/openapi/vfs/impl/VirtualFilePointerTest.java +++ b/platform/platform-tests/testSrc/com/intellij/openapi/vfs/impl/VirtualFilePointerTest.java @@ -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 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); + } + } }