From ced6ea877f6cfceaa41c4747893326d6b46c83b8 Mon Sep 17 00:00:00 2001 From: Alexey Kudravtsev Date: Thu, 14 Sep 2017 16:24:51 +0300 Subject: [PATCH] Enforce virtual file pointers leak tracking in fixtures; moved tracking code away from VFPMI to VirtualFilePointerTracker because it's test-only warn about not paired storePointers()/assertPointersAreDisposed() (memleaks otherwise from myStoredPointers) (disable pointer leak tracking in FixtureRule.kt for now because only @develar knows what's going on there) --- .../intellij/testFramework/IdeaTestCase.java | 6 - .../impl/VirtualFilePointerManagerImpl.java | 29 +---- .../vfs/impl/VirtualFilePointerTracker.java | 109 ++++++++++++++++++ .../com/intellij/testFramework/FixtureRule.kt | 6 +- .../testFramework/LightPlatformTestCase.java | 13 +-- .../testFramework/PlatformTestCase.java | 6 + .../impl/CodeInsightTestFixtureImpl.java | 14 +++ 7 files changed, 140 insertions(+), 43 deletions(-) create mode 100644 platform/testFramework/src/com/intellij/openapi/vfs/impl/VirtualFilePointerTracker.java diff --git a/java/testFramework/src/com/intellij/testFramework/IdeaTestCase.java b/java/testFramework/src/com/intellij/testFramework/IdeaTestCase.java index f3aa67a29a0f..8e00e3d8d3f0 100644 --- a/java/testFramework/src/com/intellij/testFramework/IdeaTestCase.java +++ b/java/testFramework/src/com/intellij/testFramework/IdeaTestCase.java @@ -19,8 +19,6 @@ import com.intellij.openapi.module.ModuleType; import com.intellij.openapi.module.StdModuleTypes; import com.intellij.openapi.projectRoots.Sdk; import com.intellij.openapi.roots.LanguageLevelProjectExtension; -import com.intellij.openapi.vfs.impl.VirtualFilePointerManagerImpl; -import com.intellij.openapi.vfs.pointers.VirtualFilePointerManager; import com.intellij.pom.java.LanguageLevel; import com.intellij.psi.PsiClass; import com.intellij.psi.impl.JavaPsiFacadeEx; @@ -39,16 +37,12 @@ public abstract class IdeaTestCase extends PlatformTestCase { super.setUp(); LanguageLevelProjectExtension.getInstance(getProject()).setLanguageLevel(LanguageLevel.JDK_1_6); myJavaFacade = JavaPsiFacadeEx.getInstanceEx(myProject); - VirtualFilePointerManagerImpl filePointerManager = (VirtualFilePointerManagerImpl)VirtualFilePointerManager.getInstance(); - filePointerManager.storePointers(); } @Override protected void tearDown() throws Exception { myJavaFacade = null; super.tearDown(); - VirtualFilePointerManagerImpl filePointerManager = (VirtualFilePointerManagerImpl)VirtualFilePointerManager.getInstance(); - filePointerManager.assertPointersAreDisposed(); } public final JavaPsiFacadeEx getJavaFacade() { 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 2e5506677418..edec81f4021a 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 @@ -15,6 +15,7 @@ */ package com.intellij.openapi.vfs.impl; +import com.intellij.concurrency.ConcurrentCollectionFactory; import com.intellij.openapi.Disposable; import com.intellij.openapi.application.ApplicationManager; import com.intellij.openapi.diagnostic.Logger; @@ -35,7 +36,6 @@ import com.intellij.openapi.vfs.pointers.VirtualFilePointerListener; import com.intellij.openapi.vfs.pointers.VirtualFilePointerManager; import com.intellij.util.ConcurrencyUtil; import com.intellij.util.SmartList; -import com.intellij.concurrency.ConcurrentCollectionFactory; import com.intellij.util.containers.ContainerUtil; import com.intellij.util.io.URLUtil; import com.intellij.util.messages.MessageBus; @@ -312,33 +312,8 @@ public class VirtualFilePointerManagerImpl extends VirtualFilePointerManager imp } } - private final Set myStoredPointers = ContainerUtil.newIdentityTroveSet(); - @TestOnly - public synchronized void storePointers() { - myStoredPointers.clear(); - addAllPointersTo(myStoredPointers); - } - - @TestOnly - public synchronized void assertPointersAreDisposed() { - List pointers = new ArrayList<>(); - addAllPointersTo(pointers); - try { - for (VirtualFilePointerImpl pointer : pointers) { - if (!myStoredPointers.contains(pointer)) { - pointer.throwDisposalError("Virtual pointer '" + pointer + - "' hasn't been disposed: "+pointer.getStackTrace()); - } - } - } - finally { - myStoredPointers.clear(); - } - } - - @TestOnly - private void addAllPointersTo(@NotNull Collection pointers) { + synchronized void addAllPointersTo(@NotNull Collection pointers) { List out = new ArrayList<>(); for (FilePointerPartNode root : myPointers.values()) { root.addPointersUnder(null, false, "", out); diff --git a/platform/testFramework/src/com/intellij/openapi/vfs/impl/VirtualFilePointerTracker.java b/platform/testFramework/src/com/intellij/openapi/vfs/impl/VirtualFilePointerTracker.java new file mode 100644 index 000000000000..362f4db17c9b --- /dev/null +++ b/platform/testFramework/src/com/intellij/openapi/vfs/impl/VirtualFilePointerTracker.java @@ -0,0 +1,109 @@ +/* + * Copyright 2000-2017 JetBrains s.r.o. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.intellij.openapi.vfs.impl; + +import com.intellij.openapi.util.io.FileUtil; +import com.intellij.openapi.vfs.pointers.VirtualFilePointerManager; +import com.intellij.util.containers.ContainerUtil; +import gnu.trove.TObjectHashingStrategy; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.TestOnly; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.Set; + +/** + * Tracks leaks of file pointers from {@link VirtualFilePointerManagerImpl} + * Usage: + * + * 
{@code
+ * class MyTest {
+ *   VirtualFilePointerTracker myTracker;
+ *   void setUpOrSomewhereBeforeTestExecution() {
+ *     myTracker = new VirtualFilePointerTracker(); // all virtual file pointers created by this moment are remembered
+ *   }
+ *   void tearDownOrSomewhereAfterTestExecuted() {
+ *     myTracker.assertPointersAreDisposed(); // throws if there are virtual file pointers created after setup but never disposed
+ *   }
+ * }
+ * }
+ */ +@TestOnly +public class VirtualFilePointerTracker { + private static final Set storedPointers = ContainerUtil.newIdentityTroveSet(); + private static Throwable trace; + private static boolean isTracking; // true when storePointers() was called but before assertPointersDisposed(). false otherwise + + public VirtualFilePointerTracker() { + storePointers(); + } + + private synchronized void storePointers() { + if (isTracking) { + throw new IllegalStateException("Previous test did not call assertPointersAreDisposed() - see 'Caused by:' for its stacktrace", trace); + } + trace = new Throwable(); + storedPointers.clear(); + addAllPointersTo(storedPointers); + //System.out.println("VFPT.storePointers(" + storedPointers + ")"); + isTracking = true; + } + + public synchronized void assertPointersAreDisposed() { + if (!isTracking) { + throw new IllegalStateException("Double call of assertPointersAreDisposed() - see 'Caused by:' for the previous call", trace); + } + List pointers = new ArrayList<>(); + addAllPointersTo(pointers); + //System.out.println("VFPT.assertPointersAreDisposed(" +pointers+")"); + for (int i = pointers.size() - 1; i >= 0; i--) { + VirtualFilePointerImpl pointer = pointers.get(i); + if (storedPointers.remove(pointer)) { + pointers.remove(i); + } + } + try { + Set leaked = ContainerUtil.newTroveSet(new TObjectHashingStrategy() { + @Override + public int computeHashCode(VirtualFilePointerImpl pointer) { + return FileUtil.PATH_HASHING_STRATEGY.computeHashCode(pointer.getUrl()); + } + + @Override + public boolean equals(VirtualFilePointerImpl o1, VirtualFilePointerImpl o2) { + return FileUtil.PATH_HASHING_STRATEGY.equals(o1.getUrl(), o2.getUrl()); + } + }, pointers); + leaked.removeAll(storedPointers); + + for (VirtualFilePointerImpl pointer : leaked) { + pointer.throwDisposalError("Virtual pointer '" + pointer + + "' hasn't been disposed: " + pointer.getStackTrace()); + } + } + finally { + storedPointers.clear(); + trace = new Throwable(); + isTracking = false; + } + } + + private static void addAllPointersTo(@NotNull Collection pointers) { + ((VirtualFilePointerManagerImpl)VirtualFilePointerManager.getInstance()).addAllPointersTo(pointers); + } +} diff --git a/platform/testFramework/src/com/intellij/testFramework/FixtureRule.kt b/platform/testFramework/src/com/intellij/testFramework/FixtureRule.kt index e2f4581ec6e9..7743f7c6221e 100644 --- a/platform/testFramework/src/com/intellij/testFramework/FixtureRule.kt +++ b/platform/testFramework/src/com/intellij/testFramework/FixtureRule.kt @@ -91,7 +91,8 @@ class ProjectRule(val projectDescriptor: LightProjectDescriptor = LightProjectDe } } - (VirtualFilePointerManager.getInstance() as VirtualFilePointerManagerImpl).storePointers() + // TODO uncomment and figure out where to put this statement +// (VirtualFilePointerManager.getInstance() as VirtualFilePointerManagerImpl).storePointers() return project } @@ -100,7 +101,8 @@ class ProjectRule(val projectDescriptor: LightProjectDescriptor = LightProjectDe sharedProject = null sharedModule = null (ProjectManager.getInstance() as ProjectManagerImpl).forceCloseProject(project, true) - (VirtualFilePointerManager.getInstance() as VirtualFilePointerManagerImpl).assertPointersAreDisposed() + // TODO uncomment and figure out where to put this statement +// (VirtualFilePointerManager.getInstance() as VirtualFilePointerManagerImpl).assertPointersAreDisposed() } } diff --git a/platform/testFramework/src/com/intellij/testFramework/LightPlatformTestCase.java b/platform/testFramework/src/com/intellij/testFramework/LightPlatformTestCase.java index b7ee558bb9ba..a00bd3bbfaed 100644 --- a/platform/testFramework/src/com/intellij/testFramework/LightPlatformTestCase.java +++ b/platform/testFramework/src/com/intellij/testFramework/LightPlatformTestCase.java @@ -71,10 +71,9 @@ import com.intellij.openapi.vfs.LocalFileSystem; import com.intellij.openapi.vfs.VirtualFile; import com.intellij.openapi.vfs.encoding.EncodingManager; import com.intellij.openapi.vfs.encoding.EncodingManagerImpl; -import com.intellij.openapi.vfs.impl.VirtualFilePointerManagerImpl; +import com.intellij.openapi.vfs.impl.VirtualFilePointerTracker; import com.intellij.openapi.vfs.newvfs.persistent.PersistentFS; import com.intellij.openapi.vfs.newvfs.persistent.PersistentFSImpl; -import com.intellij.openapi.vfs.pointers.VirtualFilePointerManager; import com.intellij.psi.PsiDocumentManager; import com.intellij.psi.PsiFile; import com.intellij.psi.PsiFileFactory; @@ -139,6 +138,8 @@ public abstract class LightPlatformTestCase extends UsefulTestCase implements Da PlatformTestUtil.registerProjectCleanup(LightPlatformTestCase::closeAndDeleteProject); } + private VirtualFilePointerTracker myVirtualFilePointerTracker; + /** * @return Project to be used in tests for example for project components retrieval. */ @@ -251,9 +252,6 @@ public abstract class LightPlatformTestCase extends UsefulTestCase implements Da ourSourceRoot = sourceRoot; } }); - - // project creation may make a lot of pointers, do not regard them as leak - ((VirtualFilePointerManagerImpl)VirtualFilePointerManager.getInstance()).storePointers(); } /** @@ -282,8 +280,7 @@ public abstract class LightPlatformTestCase extends UsefulTestCase implements Da myThreadTracker = new ThreadTracker(); ModuleRootManager.getInstance(ourModule).orderEntries().getAllLibrariesAndSdkClassesRoots(); - VirtualFilePointerManagerImpl filePointerManager = (VirtualFilePointerManagerImpl)VirtualFilePointerManager.getInstance(); - filePointerManager.storePointers(); + myVirtualFilePointerTracker = new VirtualFilePointerTracker(); }); } @@ -390,7 +387,7 @@ public abstract class LightPlatformTestCase extends UsefulTestCase implements Da super::tearDown, () -> myThreadTracker.checkLeak(), () -> InjectedLanguageManagerImpl.checkInjectorsAreDisposed(project), - () -> ((VirtualFilePointerManagerImpl)VirtualFilePointerManager.getInstance()).assertPointersAreDisposed() + () -> myVirtualFilePointerTracker.assertPointersAreDisposed() ).run(); } diff --git a/platform/testFramework/src/com/intellij/testFramework/PlatformTestCase.java b/platform/testFramework/src/com/intellij/testFramework/PlatformTestCase.java index 333dbb5f3389..eb03010b5c03 100644 --- a/platform/testFramework/src/com/intellij/testFramework/PlatformTestCase.java +++ b/platform/testFramework/src/com/intellij/testFramework/PlatformTestCase.java @@ -58,11 +58,14 @@ import com.intellij.openapi.vfs.JarFileSystem; import com.intellij.openapi.vfs.LocalFileSystem; import com.intellij.openapi.vfs.VfsUtil; import com.intellij.openapi.vfs.VirtualFile; +import com.intellij.openapi.vfs.impl.VirtualFilePointerManagerImpl; +import com.intellij.openapi.vfs.impl.VirtualFilePointerTracker; import com.intellij.openapi.vfs.impl.jar.JarFileSystemImpl; import com.intellij.openapi.vfs.impl.local.LocalFileSystemImpl; import com.intellij.openapi.vfs.newvfs.impl.VirtualDirectoryImpl; import com.intellij.openapi.vfs.newvfs.persistent.PersistentFS; import com.intellij.openapi.vfs.newvfs.persistent.PersistentFSImpl; +import com.intellij.openapi.vfs.pointers.VirtualFilePointerManager; import com.intellij.psi.PsiDocumentManager; import com.intellij.psi.PsiFile; import com.intellij.psi.PsiManager; @@ -125,6 +128,7 @@ public abstract class PlatformTestCase extends UsefulTestCase implements DataPro private static boolean ourPlatformPrefixInitialized; private static Set ourEternallyLivingFilesCache; private Sdk[] myOldSdks; + private VirtualFilePointerTracker myVirtualFilePointerTracker; /** * If a temp directory is reused from some previous test run, there might be cached children in its VFS. @@ -224,6 +228,7 @@ public abstract class PlatformTestCase extends UsefulTestCase implements DataPro DocumentCommitThread.getInstance().clearQueue(); UIUtil.dispatchAllInvocationEvents(); + myVirtualFilePointerTracker = new VirtualFilePointerTracker(); } public final Project getProject() { @@ -518,6 +523,7 @@ public abstract class PlatformTestCase extends UsefulTestCase implements DataPro }) .append(LightPlatformTestCase::checkEditorsReleased) .append(() -> UsefulTestCase.checkForJdkTableLeaks(myOldSdks)) + .append(() -> myVirtualFilePointerTracker.assertPointersAreDisposed()) .append(() -> { myProjectManager = null; myProject = null; diff --git a/platform/testFramework/src/com/intellij/testFramework/fixtures/impl/CodeInsightTestFixtureImpl.java b/platform/testFramework/src/com/intellij/testFramework/fixtures/impl/CodeInsightTestFixtureImpl.java index 96a67d63aced..31893fbab05f 100644 --- a/platform/testFramework/src/com/intellij/testFramework/fixtures/impl/CodeInsightTestFixtureImpl.java +++ b/platform/testFramework/src/com/intellij/testFramework/fixtures/impl/CodeInsightTestFixtureImpl.java @@ -83,18 +83,22 @@ import com.intellij.openapi.extensions.ExtensionPointName; import com.intellij.openapi.extensions.ExtensionsArea; import com.intellij.openapi.fileEditor.*; import com.intellij.openapi.fileEditor.ex.FileEditorManagerEx; +import com.intellij.openapi.fileEditor.impl.EditorHistoryManager; import com.intellij.openapi.fileEditor.impl.text.TextEditorProvider; import com.intellij.openapi.fileTypes.FileType; import com.intellij.openapi.fileTypes.FileTypeManager; import com.intellij.openapi.module.Module; +import com.intellij.openapi.module.ModuleManager; import com.intellij.openapi.progress.ProcessCanceledException; import com.intellij.openapi.project.DumbService; import com.intellij.openapi.project.Project; +import com.intellij.openapi.roots.ModuleRootManager; import com.intellij.openapi.util.*; import com.intellij.openapi.util.io.FileUtil; import com.intellij.openapi.util.text.StringUtil; import com.intellij.openapi.vcs.readOnlyHandler.ReadonlyStatusHandlerImpl; import com.intellij.openapi.vfs.*; +import com.intellij.openapi.vfs.impl.VirtualFilePointerTracker; import com.intellij.psi.*; import com.intellij.psi.impl.DebugUtil; import com.intellij.psi.impl.PsiManagerEx; @@ -162,6 +166,7 @@ public class CodeInsightTestFixtureImpl extends BaseFixture implements CodeInsig private ChooseByNameBase myChooseByNamePopup; private boolean myAllowDirt; private boolean myCaresAboutInjection = true; + private VirtualFilePointerTracker myVirtualFilePointerTracker; public CodeInsightTestFixtureImpl(@NotNull IdeaProjectTestFixture projectFixture, @NotNull TempDirTestFixture tempDirTestFixture) { myProjectFixture = projectFixture; @@ -1206,6 +1211,11 @@ public class CodeInsightTestFixtureImpl extends BaseFixture implements CodeInsig ensureIndexesUpToDate(getProject()); ((StartupManagerImpl)StartupManagerEx.getInstanceEx(getProject())).runPostStartupActivities(); }); + + for (Module module : ModuleManager.getInstance(getProject()).getModules()) { + ModuleRootManager.getInstance(module).orderEntries().getAllLibrariesAndSdkClassesRoots(); // instantiate all VFPs + } + myVirtualFilePointerTracker = new VirtualFilePointerTracker(); } @Override @@ -1233,12 +1243,16 @@ public class CodeInsightTestFixtureImpl extends BaseFixture implements CodeInsig } finally { super.tearDown(); + myVirtualFilePointerTracker.assertPointersAreDisposed(); } } private void closeOpenFiles() { PsiDocumentManager.getInstance(getProject()).commitAllDocuments(); FileEditorManagerEx.getInstanceEx(getProject()).closeAllFiles(); + for (VirtualFile file : EditorHistoryManager.getInstance(getProject()).getFiles()) { + EditorHistoryManager.getInstance(getProject()).removeFile(file); + } } @NotNull