From 41b438000b760782a5d1888d384921c37a399010 Mon Sep 17 00:00:00 2001 From: Alexey Kudravtsev Date: Tue, 22 Nov 2016 15:33:03 +0300 Subject: [PATCH] apply all highlighters via one queue to avoid races 'create new error incrementally/ remove error in setHighlightsToEditorInRange()' in incorrectly written annotators (which ignore error highlighter locality by doing visit file recursively etc) (IDEA-163273 PhpStorm: error highlighting appears and then disappear immediately) --- .../impl/DaemonRespondToChangesTest.java | 58 +++++++++++++++++-- .../daemon/impl/HighlightingSessionImpl.java | 42 +++++--------- .../impl/DefaultHighlightInfoProcessor.java | 9 +-- 3 files changed, 72 insertions(+), 37 deletions(-) diff --git a/java/java-tests/testSrc/com/intellij/codeInsight/daemon/impl/DaemonRespondToChangesTest.java b/java/java-tests/testSrc/com/intellij/codeInsight/daemon/impl/DaemonRespondToChangesTest.java index d8880bec4749..c00a6b88438b 100644 --- a/java/java-tests/testSrc/com/intellij/codeInsight/daemon/impl/DaemonRespondToChangesTest.java +++ b/java/java-tests/testSrc/com/intellij/codeInsight/daemon/impl/DaemonRespondToChangesTest.java @@ -48,11 +48,9 @@ import com.intellij.ide.GeneralSettings; import com.intellij.ide.SaveAndSyncHandlerImpl; import com.intellij.ide.highlighter.JavaFileType; import com.intellij.javaee.ExternalResourceManagerExImpl; -import com.intellij.lang.CompositeLanguage; -import com.intellij.lang.ExternalLanguageAnnotators; -import com.intellij.lang.LanguageFilter; -import com.intellij.lang.StdLanguages; +import com.intellij.lang.*; import com.intellij.lang.annotation.AnnotationHolder; +import com.intellij.lang.annotation.Annotator; import com.intellij.lang.annotation.ExternalAnnotator; import com.intellij.lang.annotation.HighlightSeverity; import com.intellij.lang.java.JavaLanguage; @@ -2389,5 +2387,57 @@ public class DaemonRespondToChangesTest extends DaemonAnalyzerTestCase { assertEquals(TextRange.from(0, document.getTextLength()), fileStatusMap.getFileDirtyScope(document, Pass.UPDATE_ALL)); }); } + + public void testAddRemoveHighlighterRaceInIncorrectAnnotatorsWhichUseFileRecursiveVisit() throws Exception { + Annotator annotator = new MyIncorrectlyRecursiveAnnotator(); + com.intellij.lang.Language java = StdFileTypes.JAVA.getLanguage(); + LanguageAnnotators.INSTANCE.addExplicitExtension(java, annotator); + try { + List list = LanguageAnnotators.INSTANCE.allForLanguage(java); + assertTrue(list.toString(), list.contains(annotator)); + configureByText(StdFileTypes.JAVA, "class X {\n" + + " int foo(Object param) {\n" + + " if (param == this) return 1;\n" + + " return 0;\n" + + " }\n" + + "}\n"); + ((EditorImpl)myEditor).getScrollPane().getViewport().setSize(1000, 1000); + assertEquals(getFile().getTextRange(), VisibleHighlightingPassFactory.calculateVisibleRange(getEditor())); + + assertEquals("XXX", assertOneElement(doHighlighting(HighlightSeverity.WARNING)).getDescription()); + + for (int i=0; i<100; i++) { + //System.out.println("i = " + i); + DaemonCodeAnalyzer.getInstance(getProject()).restart(); + List infos = doHighlighting(HighlightSeverity.WARNING); + assertEquals("XXX", assertOneElement(infos).getDescription()); + } + } + finally { + LanguageAnnotators.INSTANCE.removeExplicitExtension(java, annotator); + } + + List list = LanguageAnnotators.INSTANCE.allForLanguage(java); + assertFalse(list.toString(), list.contains(annotator)); + } + + public static class MyIncorrectlyRecursiveAnnotator implements Annotator { + Random random = new Random(); + @Override + public void annotate(@NotNull PsiElement psiElement, @NotNull AnnotationHolder holder) { + if (psiElement instanceof PsiFile) { + psiElement.accept(new JavaRecursiveElementWalkingVisitor(){ + @Override + public void visitKeyword(PsiKeyword keyword) { + if (Objects.equals(keyword.getText(), "this")) { + holder.createAnnotation(HighlightSeverity.WARNING, keyword.getTextRange(), "XXX"); + TimeoutUtil.sleep(random.nextInt(100)); + } + } + }); + } + } + } + } diff --git a/platform/analysis-impl/src/com/intellij/codeInsight/daemon/impl/HighlightingSessionImpl.java b/platform/analysis-impl/src/com/intellij/codeInsight/daemon/impl/HighlightingSessionImpl.java index 16d7f4548085..df78ef77fe0f 100644 --- a/platform/analysis-impl/src/com/intellij/codeInsight/daemon/impl/HighlightingSessionImpl.java +++ b/platform/analysis-impl/src/com/intellij/codeInsight/daemon/impl/HighlightingSessionImpl.java @@ -45,8 +45,7 @@ public class HighlightingSessionImpl implements HighlightingSession { @NotNull private final Project myProject; private final Document myDocument; private final Map myRanges2markersCache = new THashMap<>(); - private final TransferToEDTQueue myAddHighlighterInEDTQueue; - private final TransferToEDTQueue myDisposeHighlighterInEDTQueue; + private final TransferToEDTQueue myEDTQueue; private HighlightingSessionImpl(@NotNull PsiFile psiFile, @Nullable Editor editor, @@ -58,22 +57,18 @@ public class HighlightingSessionImpl implements HighlightingSession { myEditorColorsScheme = editorColorsScheme; myProject = psiFile.getProject(); myDocument = PsiDocumentManager.getInstance(myProject).getDocument(psiFile); - myDisposeHighlighterInEDTQueue = new TransferToEDTQueue<>("Dispose abandoned highlighter", highlighter -> { - highlighter.dispose(); - return true; - }, o -> myProject.isDisposed() || getProgressIndicator().isCanceled(), 200); - myAddHighlighterInEDTQueue = new TransferToEDTQueue<>("Apply highlighting results", info -> { - final EditorColorsScheme colorsScheme = getColorsScheme(); - UpdateHighlightersUtil.addHighlighterToEditorIncrementally(myProject, getDocument(), getPsiFile(), info.myRestrictRange.getStartOffset(), - info.myRestrictRange.getEndOffset(), - info.myInfo, colorsScheme, info.myGroupId, myRanges2markersCache); - + myEDTQueue = new TransferToEDTQueue<>("Apply highlighting results", runnable -> { + runnable.run(); return true; }, o -> myProject.isDisposed() || getProgressIndicator().isCanceled(), 200); } private static final Key> HIGHLIGHTING_SESSION = Key.create("HIGHLIGHTING_SESSION"); + void applyInEDT(@NotNull Runnable runnable) { + myEDTQueue.offer(runnable); + } + public static HighlightingSession getHighlightingSession(@NotNull PsiFile psiFile, @NotNull ProgressIndicator progressIndicator) { Map map = ((DaemonProgressIndicator)progressIndicator).getUserData(HIGHLIGHTING_SESSION); return map == null ? null : map.get(psiFile); @@ -135,28 +130,21 @@ public class HighlightingSessionImpl implements HighlightingSession { @NotNull TextRange priorityRange, @NotNull TextRange restrictedRange, int groupId) { - myAddHighlighterInEDTQueue.offer(new Info(info, restrictedRange, groupId)); + myEDTQueue.offer(() -> { + final EditorColorsScheme colorsScheme = getColorsScheme(); + UpdateHighlightersUtil.addHighlighterToEditorIncrementally(myProject, getDocument(), getPsiFile(), restrictedRange.getStartOffset(), + restrictedRange.getEndOffset(), + info, colorsScheme, groupId, myRanges2markersCache); + }); } void queueDisposeHighlighter(@Nullable RangeHighlighterEx highlighter) { if (highlighter == null) return; - myDisposeHighlighterInEDTQueue.offer(highlighter); - } - - private static class Info { - @NotNull private final HighlightInfo myInfo; - @NotNull private final TextRange myRestrictRange; - private final int myGroupId; - - private Info(@NotNull HighlightInfo info, @NotNull TextRange restrictRange, int groupId) { - myInfo = info; - myRestrictRange = restrictRange; - myGroupId = groupId; - } + myEDTQueue.offer(highlighter::dispose); } void waitForHighlightInfosApplied() { ApplicationManager.getApplication().assertIsDispatchThread(); - myAddHighlighterInEDTQueue.drain(); + myEDTQueue.drain(); } } diff --git a/platform/lang-impl/src/com/intellij/codeInsight/daemon/impl/DefaultHighlightInfoProcessor.java b/platform/lang-impl/src/com/intellij/codeInsight/daemon/impl/DefaultHighlightInfoProcessor.java index d5029ddf7ca1..1568ae76ed50 100644 --- a/platform/lang-impl/src/com/intellij/codeInsight/daemon/impl/DefaultHighlightInfoProcessor.java +++ b/platform/lang-impl/src/com/intellij/codeInsight/daemon/impl/DefaultHighlightInfoProcessor.java @@ -17,7 +17,6 @@ package com.intellij.codeInsight.daemon.impl; import com.intellij.codeHighlighting.Pass; import com.intellij.openapi.application.ApplicationManager; -import com.intellij.openapi.application.TransactionGuard; import com.intellij.openapi.editor.Document; import com.intellij.openapi.editor.Editor; import com.intellij.openapi.editor.colors.EditorColorsScheme; @@ -33,7 +32,6 @@ import com.intellij.psi.PsiDocumentManager; import com.intellij.psi.PsiFile; import com.intellij.psi.util.PsiUtilBase; import com.intellij.util.Alarm; -import com.intellij.util.ui.UIUtil; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -54,7 +52,7 @@ public class DefaultHighlightInfoProcessor extends HighlightInfoProcessor { final TextRange priorityIntersection = priorityRange.intersection(restrictRange); final Editor editor = session.getEditor(); - TransactionGuard.submitTransaction(project, () -> { + ((HighlightingSessionImpl)session).applyInEDT(() -> { if (modificationStamp != document.getModificationStamp()) return; if (priorityIntersection != null) { MarkupModel markupModel = DocumentMarkupModel.forDocument(document, project, true); @@ -84,7 +82,7 @@ public class DefaultHighlightInfoProcessor extends HighlightInfoProcessor { final Document document = PsiDocumentManager.getInstance(project).getDocument(psiFile); if (document == null) return; final long modificationStamp = document.getModificationStamp(); - UIUtil.invokeLaterIfNeeded(() -> { + ((HighlightingSessionImpl)session).applyInEDT(() -> { if (project.isDisposed() || modificationStamp != document.getModificationStamp()) return; EditorColorsScheme scheme = session.getColorsScheme(); @@ -116,8 +114,7 @@ public class DefaultHighlightInfoProcessor extends HighlightInfoProcessor { final Project project = psiFile.getProject(); final Document document = PsiDocumentManager.getInstance(project).getDocument(psiFile); if (document == null) return; - DaemonCodeAnalyzerEx - .processHighlights(document, project, null, range.getStartOffset(), range.getEndOffset(), existing -> { + DaemonCodeAnalyzerEx.processHighlights(document, project, null, range.getStartOffset(), range.getEndOffset(), existing -> { if (existing.isBijective() && existing.getGroup() == Pass.UPDATE_ALL && range.equalsToRange(existing.getActualStartOffset(), existing.getActualEndOffset())) {