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 717d468f158f..465df1dbf618 100644 --- a/platform/core-impl/src/com/intellij/psi/impl/PsiDocumentManagerBase.java +++ b/platform/core-impl/src/com/intellij/psi/impl/PsiDocumentManagerBase.java @@ -1186,11 +1186,19 @@ public abstract class PsiDocumentManagerBase extends PsiDocumentManager implemen VirtualFile virtualFile = FileDocumentManager.getInstance().getFile(document); if (virtualFile != null) { FileManager fileManager = getFileManager(); - FileViewProvider viewProvider = fileManager.findCachedViewProvider(virtualFile); - if (viewProvider != null) { + List viewProviders = fileManager.findCachedViewProviders(virtualFile); + boolean isWriteAccess = ApplicationManager.getApplication().isWriteAccessAllowed(); + if (!viewProviders.isEmpty()) { // we can end up outside write action here if the document has forUseInNonAWTThread=true - ApplicationManager.getApplication().runWriteAction(ExternalChangeActionUtil.externalChangeAction(() -> - ((AbstractFileViewProvider)viewProvider).onContentReload())); + ApplicationManager.getApplication().runWriteAction(ExternalChangeActionUtil.externalChangeAction(() -> { + List effectiveViewProviders = + isWriteAccess ? viewProviders + : fileManager.findCachedViewProviders(virtualFile); // new view providers could appear concurrently + + for (FileViewProvider viewProvider : effectiveViewProviders) { + ((AbstractFileViewProvider)viewProvider).onContentReload(); + } + })); } else if (FileIndexFacade.getInstance(myProject).isInContent(virtualFile)) { ApplicationManager.getApplication().runWriteAction(ExternalChangeActionUtil.externalChangeAction(() -> diff --git a/platform/ide-core-impl/src/com/intellij/psi/impl/DocumentCommitThread.kt b/platform/ide-core-impl/src/com/intellij/psi/impl/DocumentCommitThread.kt index 4f5754342b12..f780a119abe9 100644 --- a/platform/ide-core-impl/src/com/intellij/psi/impl/DocumentCommitThread.kt +++ b/platform/ide-core-impl/src/com/intellij/psi/impl/DocumentCommitThread.kt @@ -10,6 +10,7 @@ import com.intellij.openapi.application.* import com.intellij.openapi.components.Service import com.intellij.openapi.components.service import com.intellij.openapi.diagnostic.logger +import com.intellij.openapi.diagnostic.trace import com.intellij.openapi.editor.Document import com.intellij.openapi.editor.ex.DocumentEx import com.intellij.openapi.fileEditor.FileDocumentManager @@ -22,7 +23,6 @@ import com.intellij.openapi.util.ProperTextRange import com.intellij.openapi.util.TextRange import com.intellij.openapi.util.registry.Registry import com.intellij.openapi.util.text.StringUtil -import com.intellij.openapi.vfs.VirtualFile import com.intellij.psi.* import com.intellij.psi.text.BlockSupport import com.intellij.util.SmartList @@ -199,12 +199,12 @@ class DocumentCommitThread : DocumentCommitProcessor, Disposable { val finishProcessors = SmartList() val reparseInjectedProcessors = SmartList() + LOG.trace { "commitUnderProgress: ${task.myReason}, $document, synchronously: $synchronously " } - val psiManager = PsiManagerEx.getInstanceEx(project) - val virtualFile = FileDocumentManager.getInstance().getFile(document) - val viewProviders = findViewProvidersForCommit(psiManager, virtualFile) + val viewProviders = findViewProvidersForCommit(document, project) if (viewProviders.isEmpty()) { finishProcessors.add(handleCommitWithoutPsi(task, documentManager)) + task.cachedViewProviders = emptyList() } else { // While we were messing around transferring things to background thread, the ViewProviders can become obsolete @@ -239,6 +239,13 @@ class DocumentCommitThread : DocumentCommitProcessor, Disposable { // this document was not referenced by anyone, hence we don't need to perform a write action val document = task.myDocumentRef.get() ?: return@task + + if (!synchronously && newViewProvidersWereConcurrentlyAdded(document, task.cachedViewProviders, project)) { + // add a document back to the queue + commitAsynchronously(project, documentManager, document, "Re-added back because of new view providers", task.myCreationModality) + return@task + } + val success = documentManager.finishCommit(document, finishProcessors, reparseInjectedProcessors, synchronously, task.myReason) if (synchronously) { assert(success) @@ -246,31 +253,55 @@ class DocumentCommitThread : DocumentCommitProcessor, Disposable { if (synchronously || success) { assert(!documentManager.isInUncommittedSet(document)) } - if (!success && viewProviders.isEventSystemEnabled()) { + if (!success && task.cachedViewProviders.isEventSystemEnabled()) { // add a document back to the queue commitAsynchronously(project, documentManager, document, "Re-added back", task.myCreationModality) } } } - private fun findViewProvidersForCommit( - psiManager: PsiManagerEx, - virtualFile: VirtualFile?, - ): List { - if (virtualFile == null) { - return emptyList() - } + private fun findViewProvidersForCommit(document: Document, project: Project): List { + val psiManager = PsiManagerEx.getInstanceEx(project) + val virtualFile = FileDocumentManager.getInstance().getFile(document) ?: return emptyList() if (isSharedSourceSupportEnabled(psiManager.project)) { - val providers = psiManager.fileManagerEx.findCachedViewProviders(virtualFile) - if (providers.isNotEmpty()) { - return providers + val cached = psiManager.fileManagerEx.findCachedViewProviders(virtualFile) + if (cached.isNotEmpty()) { + return cached } - // no providers mean that they might be collected. - // so let's try and find at least one with the help of the following line. + } + return listOfNotNull(psiManager.findViewProvider(virtualFile)) + } + + private fun newViewProvidersWereConcurrentlyAdded( + document: Document, + committedViewProviders: List, + project: Project, + ): Boolean { + val currentProviders = findViewProvidersForCommit(document, project) + + if (committedViewProviders.size != currentProviders.size) { + LOG.trace { "Concurrent view provider modification detected. Was: ${committedViewProviders.size}, Now: ${currentProviders.size}. Adding document back to the queue. $document" } + return true } - return listOfNotNull(psiManager.findViewProvider(virtualFile)) + if (committedViewProviders.size == 1) { + if (committedViewProviders.first() == currentProviders.first()) { + return false + } + else { + LOG.trace { "Concurrent view provider modification detected: view provider was changed to another one. Adding document back to the queue. $document" } + return true + } + } + + if (committedViewProviders.toSet().containsAll(currentProviders)) { + return false + } + else { + LOG.trace { "Concurrent view provider modification detected. Adding document back to the queue. $document" } + return true + } } override fun toString(): String = "Document commit thread; application: ${ApplicationManager.getApplication()}; isDisposed: $isDisposed" @@ -312,7 +343,9 @@ class DocumentCommitThread : DocumentCommitProcessor, Disposable { val myLastCommittedText: CharSequence // store initial document modification sequence here to check if it changed later before commit in EDT private val myModificationSequence: Int - @Volatile var cachedViewProviders: List? = null + + /** initialized under read-action in commitUnderProgress */ + @Volatile lateinit var cachedViewProviders: List constructor( project: Project, diff --git a/platform/lang-impl/testSources/com/intellij/psi/impl/file/impl/CommitMultiverseFileTest.kt b/platform/lang-impl/testSources/com/intellij/psi/impl/file/impl/CommitMultiverseFileTest.kt index a1ca015f3d63..d20ab9e7c3d4 100644 --- a/platform/lang-impl/testSources/com/intellij/psi/impl/file/impl/CommitMultiverseFileTest.kt +++ b/platform/lang-impl/testSources/com/intellij/psi/impl/file/impl/CommitMultiverseFileTest.kt @@ -4,21 +4,24 @@ package com.intellij.psi.impl.file.impl import com.intellij.codeInsight.multiverse.ProjectModelContextBridge import com.intellij.openapi.application.readAction import com.intellij.openapi.application.writeAction +import com.intellij.openapi.command.CommandProcessor import com.intellij.openapi.fileEditor.FileDocumentManager import com.intellij.platform.testFramework.junit5.projectStructure.fixture.withSharedSourceEnabled -import com.intellij.psi.PsiDocumentManager -import com.intellij.psi.PsiElement -import com.intellij.psi.PsiElementVisitor -import com.intellij.psi.PsiFile +import com.intellij.psi.* import com.intellij.psi.impl.PsiManagerEx import com.intellij.psi.util.PsiUtilCore import com.intellij.testFramework.IndexingTestUtil import com.intellij.testFramework.common.timeoutRunBlocking import com.intellij.testFramework.junit5.TestApplication -import com.intellij.testFramework.junit5.fixture.moduleFixture -import com.intellij.testFramework.junit5.fixture.projectFixture -import com.intellij.testFramework.junit5.fixture.virtualFileFixture +import com.intellij.testFramework.junit5.fixture.* +import kotlinx.coroutines.async +import kotlinx.coroutines.awaitAll +import kotlinx.coroutines.delay +import org.junit.jupiter.api.Assertions +import org.junit.jupiter.api.RepeatedTest import org.junit.jupiter.api.Test +import kotlin.time.Duration.Companion.milliseconds +import kotlin.time.Duration.Companion.seconds @TestApplication internal class CommitMultiverseFileTest { @@ -29,14 +32,21 @@ internal class CommitMultiverseFileTest { private val module2 = projectFixture.moduleFixture("CommitMultiverseFileTest_src2") private val sourceRoot = sharedSourceRootFixture(module1, module2) + + private val project by projectFixture + + private val psiManager by lazy { PsiManagerEx.getInstanceEx(project) } + + private val contextBridge by lazy { ProjectModelContextBridge.getInstance(project) } + private val context1 by lazy { contextBridge.getContext(module1.get())!! } + private val context2 by lazy { contextBridge.getContext(module2.get())!! } } private val virtualFile by sourceRoot.virtualFileFixture("TestCommon.java", "class A {}") + private val files by sourceRoot.fileFixtures(50) @Test fun `test commit document reparses both psi versions`() = timeoutRunBlocking { - val project = projectFixture.get() - IndexingTestUtil.waitUntilIndexesAreReady(project) val (psiFile1, psiFile2) = readAction { @@ -70,6 +80,73 @@ internal class CommitMultiverseFileTest { } } + /** + * This test checks a situation: + * a virtual file has a PSI file for context A. + * The file text gets updated and is being asynchronously committed. + * At the same time, another read-action requests a PSI file of this file with context B. + * The requested PSI file must stay in sync with the first PSI file. + * + * When commit infra is not ready for this, around 10% of test runs fail. + */ + @RepeatedTest(100) + fun `test commit document and requesting psi for another context at the same time`() = timeoutRunBlocking(20.seconds) { + IndexingTestUtil.waitUntilIndexesAreReady(project) + + // preparing psi files + val psiFiles1 = readAction { + files.map { psiManager.findFile(it, context1)!! } + } + + readAction { + for (file in psiFiles1) { + ensureParsed(file) + } + } + + val documents = readAction { + files.map { FileDocumentManager.getInstance().getDocument(it)!! } + } + + // adding long suffix to all files + writeAction { + for (file in files) { + Assertions.assertEquals(1, psiManager.fileManagerEx.findCachedViewProviders(file).size) + } + + documents.forEach { + CommandProcessor.getInstance().executeCommand(project, { + it.insertString(it.textLength, longText) + }, null, null) + } + } + + // requesting another psi file version, trying to do that concurrently with async commit + val psiFiles2 = files.asReversed().mapIndexed { index, file -> + async { + delay(index.milliseconds) // waiting a bit to increase the chance of clashing between async commit and file request + readAction { + val psi = psiManager.findFile(file, context2)!! + ensureParsed(psi) + psi + } + } + }.awaitAll() + + // waiting for commit to finish + while (PsiDocumentManager.getInstance(project).hasUncommitedDocuments()) { + delay(50) + } + + // ensuring everything is in sync + readAction { + for (file in psiFiles1 + psiFiles2) { + PsiUtilCore.ensureValid(file) + file.viewProvider.contentsSynchronized() + } + } + } + private fun ensureParsed(file: PsiFile) { file.accept(object : PsiElementVisitor() { override fun visitElement(element: PsiElement) { @@ -77,4 +154,32 @@ internal class CommitMultiverseFileTest { } }) } +} + +/** + * + * class A1 { + * class A2 { + * class A3 { + * ... + * } + * } + * } + */ +private val longText = buildString { + val times = 20 + appendLine() + repeat(times) { + appendLine("class A$it {") + } + repeat(times) { + appendLine("}") + } +} + +private fun TestFixture.fileFixtures(number: Int) = testFixture { + val files = (0..number).map { + virtualFileFixture("file$it.java", "class A {}").init() + } + initialized(files) {} } \ No newline at end of file