From 09852a929be2c6b2ec819667c235fd930fbb6b31 Mon Sep 17 00:00:00 2001 From: Alexey Kudravtsev Date: Mon, 26 Oct 2015 14:05:04 +0300 Subject: [PATCH] better diagnostics for rogue after commit listeners (which go and register another after commit handler inside) --- .../psi/impl/PsiDocumentManagerBase.java | 47 ++++---- .../psi/impl/PsiDocumentManagerImplTest.java | 100 ++++++++++++------ 2 files changed, 94 insertions(+), 53 deletions(-) diff --git a/platform/core-impl/src/com/intellij/psi/impl/PsiDocumentManagerBase.java b/platform/core-impl/src/com/intellij/psi/impl/PsiDocumentManagerBase.java index 9d2ca1dbb5a9..eb18a6dd2fd3 100644 --- a/platform/core-impl/src/com/intellij/psi/impl/PsiDocumentManagerBase.java +++ b/platform/core-impl/src/com/intellij/psi/impl/PsiDocumentManagerBase.java @@ -40,10 +40,7 @@ import com.intellij.openapi.progress.ProgressIndicator; import com.intellij.openapi.progress.ProgressManager; import com.intellij.openapi.project.Project; import com.intellij.openapi.roots.FileIndexFacade; -import com.intellij.openapi.util.Computable; -import com.intellij.openapi.util.Disposer; -import com.intellij.openapi.util.Key; -import com.intellij.openapi.util.Ref; +import com.intellij.openapi.util.*; import com.intellij.openapi.vfs.VirtualFile; import com.intellij.psi.*; import com.intellij.psi.impl.smartPointers.SmartPointerManagerImpl; @@ -75,7 +72,7 @@ public abstract class PsiDocumentManagerBase extends PsiDocumentManager implemen protected final Set myUncommittedDocuments = ContainerUtil.newConcurrentSet(); private final Map myUncommittedInfos = ContainerUtil.newConcurrentMap(); protected boolean myStopTrackingDocuments; - protected boolean myPerformBackgroundCommit = true; + private boolean myPerformBackgroundCommit = true; private volatile boolean myIsCommitInProgress; private final PsiToDocumentSynchronizer mySynchronizer; @@ -164,7 +161,6 @@ public abstract class PsiDocumentManagerBase extends PsiDocumentManager implemen return ((PsiManagerEx)myPsiManager).getFileManager().findFile(virtualFile); } - @Nullable @Override public Document getDocument(@NotNull PsiFile file) { if (file instanceof PsiBinaryFile) return null; @@ -263,6 +259,9 @@ public abstract class PsiDocumentManagerBase extends PsiDocumentManager implemen } return true; } + + checkWeAreOutsideAfterCommitHandler(); + actionsWhenAllDocumentsAreCommitted.put(key, action); return false; } @@ -484,6 +483,8 @@ public abstract class PsiDocumentManagerBase extends PsiDocumentManager implemen @Override public boolean performWhenAllCommitted(@NotNull final Runnable action) { ApplicationManager.getApplication().assertIsDispatchThread(); + checkWeAreOutsideAfterCommitHandler(); + assert !myProject.isDisposed() : "Already disposed: " + myProject; if (myUncommittedDocuments.isEmpty()) { action.run(); @@ -525,22 +526,30 @@ public abstract class PsiDocumentManagerBase extends PsiDocumentManager implemen } if (!hasUncommitedDocuments() && !actionsWhenAllDocumentsAreCommitted.isEmpty()) { - List keys = new ArrayList(actionsWhenAllDocumentsAreCommitted.keySet()); - for (Object key : keys) { - Runnable action = actionsWhenAllDocumentsAreCommitted.remove(key); + List> entries = new ArrayList>(new LinkedHashMap(actionsWhenAllDocumentsAreCommitted).entrySet()); + weAreInsideAfterCommitHandler(); + + for (Map.Entry entry : entries) { + Runnable action = entry.getValue(); + Object key = entry.getKey(); try { myDocumentCommitProcessor.log("Running after commit runnable: ", null, false, key, action); - int before = actionsWhenAllDocumentsAreCommitted.size(); action.run(); - int after = actionsWhenAllDocumentsAreCommitted.size(); - if (before != after) { - LOG.error("You must not call performWhenAllCommitted() from within after-commit handler " + action + ": "+action.getClass()); - } } catch (Throwable e) { LOG.error("During running "+action, e); } } + actionsWhenAllDocumentsAreCommitted.clear(); + } + } + + private void weAreInsideAfterCommitHandler() { + actionsWhenAllDocumentsAreCommitted.put(PERFORM_ALWAYS_KEY, EmptyRunnable.getInstance()); // to prevent listeners from registering new actions during firing + } + private void checkWeAreOutsideAfterCommitHandler() { + if (actionsWhenAllDocumentsAreCommitted.get(PERFORM_ALWAYS_KEY) == EmptyRunnable.getInstance()) { + throw new IncorrectOperationException("You must not call performWhenAllCommitted()/cancelAndRunWhenCommitted() from within after-commit handler"); } } @@ -929,12 +938,12 @@ public abstract class PsiDocumentManagerBase extends PsiDocumentManager implemen } private static class UncommittedInfo extends DocumentAdapter implements PrioritizedInternalDocumentListener { - final DocumentImpl myOriginal; - final FrozenDocument myFrozen; - final List myEvents = ContainerUtil.newArrayList(); - final ConcurrentMap myFrozenWindows = ContainerUtil.newConcurrentMap(); + private final DocumentImpl myOriginal; + private final FrozenDocument myFrozen; + private final List myEvents = ContainerUtil.newArrayList(); + private final ConcurrentMap myFrozenWindows = ContainerUtil.newConcurrentMap(); - public UncommittedInfo(DocumentImpl original) { + private UncommittedInfo(DocumentImpl original) { myOriginal = original; myFrozen = original.freeze(); myOriginal.addDocumentListener(this); diff --git a/platform/platform-tests/testSrc/com/intellij/psi/impl/PsiDocumentManagerImplTest.java b/platform/platform-tests/testSrc/com/intellij/psi/impl/PsiDocumentManagerImplTest.java index c075a47fac85..e03c3f6c45e1 100644 --- a/platform/platform-tests/testSrc/com/intellij/psi/impl/PsiDocumentManagerImplTest.java +++ b/platform/platform-tests/testSrc/com/intellij/psi/impl/PsiDocumentManagerImplTest.java @@ -45,8 +45,10 @@ import com.intellij.testFramework.LeakHunter; import com.intellij.testFramework.LightVirtualFile; import com.intellij.testFramework.PlatformTestCase; import com.intellij.testFramework.PlatformTestUtil; +import com.intellij.util.IncorrectOperationException; import com.intellij.util.concurrency.Semaphore; import com.intellij.util.ui.UIUtil; +import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import javax.swing.*; @@ -94,7 +96,7 @@ public class PsiDocumentManagerImplTest extends PlatformTestCase { VirtualFile vFile = createFile(); final PsiFile file = new MockPsiFile(vFile, getPsiManager()); - final Document document = getPsiDocumentManager().getDocument(file); + final Document document = getDocument(file); assertNotNull(document); assertSame(document, FileDocumentManager.getInstance().getDocument(vFile)); } @@ -111,7 +113,7 @@ public class PsiDocumentManagerImplTest extends PlatformTestCase { public void testDocumentGced() throws Exception { VirtualFile vFile = getVirtualFile(createTempFile("txt", "abc")); PsiDocumentManagerImpl documentManager = getPsiDocumentManager(); - long id = System.identityHashCode(documentManager.getDocument(getPsiManager().findFile(vFile))); + long id = System.identityHashCode(documentManager.getDocument(findFile(vFile))); documentManager.commitAllDocuments(); UIUtil.dispatchAllInvocationEvents(); @@ -121,28 +123,31 @@ public class PsiDocumentManagerImplTest extends PlatformTestCase { LeakHunter.checkLeak(documentManager, DocumentImpl.class); LeakHunter.checkLeak(documentManager, PsiFileImpl.class, psiFile -> psiFile.getViewProvider().getVirtualFile().getFileSystem() instanceof LocalFileSystem); - //Class.forName("com.intellij.util.ProfilingUtil").getDeclaredMethod("forceCaptureMemorySnapshot").invoke(null); for (int i = 0; i < 1000; i++) { PlatformTestUtil.tryGcSoftlyReachableObjects(); UIUtil.dispatchAllInvocationEvents(); - if (documentManager.getCachedDocument(getPsiManager().findFile(vFile)) == null) break; + if (documentManager.getCachedDocument(findFile(vFile)) == null) break; System.gc(); } - assertNull(documentManager.getCachedDocument(getPsiManager().findFile(vFile))); + assertNull(documentManager.getCachedDocument(findFile(vFile))); - Document newDoc = documentManager.getDocument(getPsiManager().findFile(vFile)); + Document newDoc = documentManager.getDocument(findFile(vFile)); assertTrue(id != System.identityHashCode(newDoc)); } + private PsiFile findFile(@NotNull VirtualFile vFile) { + return getPsiManager().findFile(vFile); + } + public void testGetUncommittedDocuments_noDocuments() throws Exception { assertEquals(0, getPsiDocumentManager().getUncommittedDocuments().length); } public void testGetUncommittedDocuments_documentChanged_DontProcessEvents() throws Exception { - final PsiFile file = getPsiManager().findFile(createFile()); + final PsiFile file = findFile(createFile()); - final Document document = getPsiDocumentManager().getDocument(file); + final Document document = getDocument(file); WriteCommandAction.runWriteCommandAction(null, () -> { getPsiDocumentManager().getSynchronizer().performAtomically(file, () -> changeDocument(document, getPsiDocumentManager())); @@ -164,19 +169,22 @@ public class PsiDocumentManagerImplTest extends PlatformTestCase { } public void testCommitDocument_RemovesFromUncommittedList() throws Exception { - PsiFile file = getPsiManager().findFile(createFile()); + PsiFile file = findFile(createFile()); - final Document document = getPsiDocumentManager().getDocument(file); + final Document document = getDocument(file); WriteCommandAction.runWriteCommandAction(null, () -> { changeDocument(document, getPsiDocumentManager()); }); - getPsiDocumentManager().commitDocument(document); assertEquals(0, getPsiDocumentManager().getUncommittedDocuments().length); } + private Document getDocument(PsiFile file) { + return getPsiDocumentManager().getDocument(file); + } + private static void changeDocument(Document document, PsiDocumentManagerImpl manager) { DocumentEventImpl event = new DocumentEventImpl(document, 0, "", "", document.getModificationStamp(), false); manager.beforeDocumentChange(event); @@ -184,9 +192,9 @@ public class PsiDocumentManagerImplTest extends PlatformTestCase { } public void testCommitAllDocument_RemovesFromUncommittedList() throws Exception { - PsiFile file = getPsiManager().findFile(createFile()); + PsiFile file = findFile(createFile()); - final Document document = getPsiDocumentManager().getDocument(file); + final Document document = getDocument(file); WriteCommandAction.runWriteCommandAction(null, () -> { changeDocument(document, getPsiDocumentManager()); @@ -198,9 +206,9 @@ public class PsiDocumentManagerImplTest extends PlatformTestCase { } public void testDocumentFromAlienProjectDoesNotEndUpInMyUncommittedList() throws Exception { - PsiFile file = getPsiManager().findFile(createFile()); + PsiFile file = findFile(createFile()); - final Document document = getPsiDocumentManager().getDocument(file); + final Document document = getDocument(file); File temp = createTempDirectory(); final Project alienProject = createProject(temp + "/alien.ipr", DebugUtil.currentStackTrace()); @@ -216,7 +224,6 @@ public class PsiDocumentManagerImplTest extends PlatformTestCase { final PsiFile alienFile = alienManager.findFile(alienVirt); final PsiDocumentManagerImpl alienDocManager = (PsiDocumentManagerImpl)PsiDocumentManager.getInstance(alienProject); final Document alienDocument = alienDocManager.getDocument(alienFile); - //alienDocument.putUserData(CACHED_VIEW_PROVIDER, new MockFileViewProvider(alienFile)); assertEquals(0, alienDocManager.getUncommittedDocuments().length); assertEquals(0, getPsiDocumentManager().getUncommittedDocuments().length); @@ -244,10 +251,10 @@ public class PsiDocumentManagerImplTest extends PlatformTestCase { } public void testCommitInBackground() { - PsiFile file = getPsiManager().findFile(createFile()); + PsiFile file = findFile(createFile()); assertNotNull(file); assertTrue(file.isPhysical()); - final Document document = getPsiDocumentManager().getDocument(file); + final Document document = getDocument(file); assertNotNull(document); final Semaphore semaphore = new Semaphore(); @@ -316,9 +323,9 @@ public class PsiDocumentManagerImplTest extends PlatformTestCase { public void testDocumentFromAlienProjectGetsCommittedInBackground() throws Exception { LightVirtualFile virtualFile = createFile(); - PsiFile file = getPsiManager().findFile(virtualFile); + PsiFile file = findFile(virtualFile); - final Document document = getPsiDocumentManager().getDocument(file); + final Document document = getDocument(file); File temp = createTempDirectory(); final Project alienProject = createProject(temp + "/alien.ipr", DebugUtil.currentStackTrace()); @@ -380,26 +387,26 @@ public class PsiDocumentManagerImplTest extends PlatformTestCase { public void testFileChangesToText() throws IOException { VirtualFile vFile = getVirtualFile(createTempFile("a.txt", "abc")); - PsiFile psiFile = getPsiManager().findFile(vFile); - Document document = getPsiDocumentManager().getDocument(psiFile); + PsiFile psiFile = findFile(vFile); + Document document = getDocument(psiFile); rename(vFile, "a.xml"); assertFalse(psiFile.isValid()); - assertNotSame(psiFile, getPsiManager().findFile(vFile)); - psiFile = getPsiManager().findFile(vFile); + assertNotSame(psiFile, findFile(vFile)); + psiFile = findFile(vFile); assertSame(document, FileDocumentManager.getInstance().getDocument(vFile)); - assertSame(document, getPsiDocumentManager().getDocument(psiFile)); + assertSame(document, getDocument(psiFile)); } public void testFileChangesToBinary() throws IOException { VirtualFile vFile = getVirtualFile(createTempFile("a.txt", "abc")); - PsiFile psiFile = getPsiManager().findFile(vFile); - Document document = getPsiDocumentManager().getDocument(psiFile); + PsiFile psiFile = findFile(vFile); + Document document = getDocument(psiFile); rename(vFile, "a.zip"); assertFalse(psiFile.isValid()); - psiFile = getPsiManager().findFile(vFile); + psiFile = findFile(vFile); assertInstanceOf(psiFile, PsiBinaryFile.class); assertNoFileDocumentMapping(vFile, psiFile, document); @@ -408,12 +415,12 @@ public class PsiDocumentManagerImplTest extends PlatformTestCase { public void testFileBecomesTooLarge() throws Exception { VirtualFile vFile = getVirtualFile(createTempFile("a.txt", "abc")); - PsiFile psiFile = getPsiManager().findFile(vFile); - Document document = getPsiDocumentManager().getDocument(psiFile); + PsiFile psiFile = findFile(vFile); + Document document = getDocument(psiFile); makeFileTooLarge(vFile); assertFalse(psiFile.isValid()); - psiFile = getPsiManager().findFile(vFile); + psiFile = findFile(vFile); assertInstanceOf(psiFile, PsiLargeFile.class); assertNoFileDocumentMapping(vFile, psiFile, document); @@ -424,7 +431,7 @@ public class PsiDocumentManagerImplTest extends PlatformTestCase { assertNull(FileDocumentManager.getInstance().getDocument(vFile)); assertNull(FileDocumentManager.getInstance().getFile(document)); assertNull(getPsiDocumentManager().getPsiFile(document)); - assertNull(getPsiDocumentManager().getDocument(psiFile)); + assertNull(getDocument(psiFile)); } private void makeFileTooLarge(final VirtualFile vFile) throws Exception { @@ -437,8 +444,8 @@ public class PsiDocumentManagerImplTest extends PlatformTestCase { public void testCommitDocumentInModalDialog() throws IOException { VirtualFile vFile = getVirtualFile(createTempFile("a.txt", "abc")); - PsiFile psiFile = getPsiManager().findFile(vFile); - final Document document = getPsiDocumentManager().getDocument(psiFile); + PsiFile psiFile = findFile(vFile); + final Document document = getDocument(psiFile); final DialogWrapper dialog = new DialogWrapper(getProject()) { @Nullable @@ -513,4 +520,29 @@ public class PsiDocumentManagerImplTest extends PlatformTestCase { FileDocumentManager.getInstance().saveDocument(document); assertEquals("-1\n2\n3\n", VfsUtilCore.loadText(file)); } + + public void testPerformWhenAllCommittedMustNotNest() { + PsiFile file = findFile(createFile()); + assertNotNull(file); + assertTrue(file.isPhysical()); + final Document document = getDocument(file); + assertNotNull(document); + + WriteCommandAction.runWriteCommandAction(null, () -> { + document.insertString(0, "class X {}"); + }); + + getPsiDocumentManager().performWhenAllCommitted(() -> { + try { + getPsiDocumentManager().performWhenAllCommitted(() -> { + + }); + fail("Must fail"); + } + catch (IncorrectOperationException ignored) { + } + }); + getPsiDocumentManager().commitAllDocuments(); + assertTrue(getPsiDocumentManager().isCommitted(document)); + } }