avoid datarace in editor creation to fix IDEA-334190 Some .kt files are not highlighted from time to time in perf tests

AsyncEditorLoader used to store `null` to the editor user data, making `isEditorLoaded()` return `true`, then store not-null there, making `isEditorLoaded()` return `false`, then store `null` again, on loading finish. This created datarace when daemon tried to start highlighting (because it thought the editor was loaded) but then canceled itself. Now the transitions are: empty list(not loaded) -> non empty list of delayed tasks (still not loaded) -> LOADED GitOrigin-RevId: 171d2d48f30fd87dae3401f0493670717ba5d86f
2026-09-27 10:03:11 +07:00 · 2024-02-22 18:56:32 +00:00
parent c93a41c727
commit 3f6db3628a
6 changed files with 169 additions and 66 deletions
@@ -18,6 +18,7 @@ import com.intellij.openapi.progress.ProcessCanceledException
 import com.intellij.openapi.progress.ProgressManager
 import com.intellij.openapi.project.Project
 import com.intellij.openapi.vfs.VirtualFile
+import kotlinx.coroutines.Deferred
 import java.awt.event.ComponentAdapter
 import java.awt.event.ComponentEvent
 import java.util.concurrent.CancellationException
@@ -39,8 +40,9 @@ open class PsiAwareTextEditorImpl : TextEditorImpl {

  internal constructor(project: Project,
                       file: VirtualFile,
-                       asyncLoader: AsyncEditorLoader,
-                       editor: EditorImpl) : super(project = project, file = file, editor = editor, asyncLoader = asyncLoader)
+                       provider: TextEditorProvider,
+                       editor: EditorImpl,
+                       task: Deferred<Unit>) : super(project = project, file = file, editor = editor, provider = provider, asyncLoader = createAsyncEditorLoader(provider, project, editor, file, task))

  override fun createEditorComponent(project: Project, file: VirtualFile, editor: EditorImpl): TextEditorComponent {
    val component = PsiAwareTextEditorComponent(project = project, file = file, textEditor = this, editor = editor)
@@ -23,7 +23,6 @@ import com.intellij.openapi.editor.impl.EditorGutterLayout
 import com.intellij.openapi.extensions.ExtensionPointName
 import com.intellij.openapi.fileEditor.*
 import com.intellij.openapi.fileEditor.impl.text.AsyncEditorLoader.Companion.isEditorLoaded
-import com.intellij.openapi.fileEditor.impl.text.TextEditorImpl.Companion.createAsyncEditorLoader
 import com.intellij.openapi.project.Project
 import com.intellij.openapi.util.WriteExternalException
 import com.intellij.openapi.vfs.VirtualFile
@@ -45,8 +44,6 @@ open class PsiAwareTextEditorProvider : TextEditorProvider(), AsyncFileEditorPro
  }

  override suspend fun createEditorBuilder(project: Project, file: VirtualFile, document: Document?): AsyncFileEditorProvider.Builder {
-    val asyncLoader = createAsyncEditorLoader(provider = this, project = project)
-
    val effectiveDocument = if (document == null) {
      val fileDocumentManager = serviceAsync<FileDocumentManager>()
      fileDocumentManager.getCachedDocument(file) ?: readActionBlocking {
@@ -76,7 +73,7 @@ open class PsiAwareTextEditorProvider : TextEditorProvider(), AsyncFileEditorPro

      val editorDeferred = CompletableDeferred<EditorEx>()

-      val task = asyncLoader.coroutineScope.async(CoroutineName("TextEditorInitializer")) {
+      val task = TextEditorImpl.editorLoaderScope(project).async(CoroutineName("call TextEditorInitializers")) {
        val editorSupplier = suspend { editorDeferred.await() }
        val highlighterReady = suspend { highlighterDeferred.join() }

@@ -122,9 +119,8 @@ open class PsiAwareTextEditorProvider : TextEditorProvider(), AsyncFileEditorPro
        override fun build(): FileEditor {
          val editor = factory.createMainEditor(effectiveDocument, project, file, highlighter)
          editor.gutterComponentEx.setInitialIconAreaWidth(EditorGutterLayout.getInitialGutterWidth())
-          val textEditor = PsiAwareTextEditorImpl(project = project, file = file, editor = editor, asyncLoader = asyncLoader)
-          editorDeferred.complete(textEditor.editor)
-          asyncLoader.start(textEditor = textEditor, tasks = listOf(task))
+          editorDeferred.complete(editor)
+          val textEditor = PsiAwareTextEditorImpl(project = project, file = file, provider = this@PsiAwareTextEditorProvider, editor = editor, task = task)
          return textEditor
        }
      }
@@ -8,7 +8,10 @@ import com.intellij.openapi.application.ModalityState
 import com.intellij.openapi.application.asContextElement
 import com.intellij.openapi.editor.Editor
 import com.intellij.openapi.editor.ex.EditorEx
+import com.intellij.openapi.editor.impl.EditorImpl
+import com.intellij.openapi.fileEditor.FileEditorManager
 import com.intellij.openapi.fileEditor.FileEditorStateLevel
+import com.intellij.openapi.fileEditor.TextEditor
 import com.intellij.openapi.project.Project
 import com.intellij.openapi.util.Key
 import com.intellij.openapi.vfs.VirtualFile
@@ -20,7 +23,6 @@ import com.intellij.util.concurrency.annotations.RequiresReadLock
 import com.intellij.util.ui.AsyncProcessIcon
 import kotlinx.coroutines.*
 import org.jetbrains.annotations.ApiStatus.Internal
-import java.util.*
 import java.util.concurrent.atomic.AtomicReference
 import javax.swing.JComponent
 import javax.swing.event.ChangeEvent
@@ -28,29 +30,56 @@ import kotlin.coroutines.resume
 import kotlin.time.Duration
 import kotlin.time.Duration.Companion.milliseconds

-private val ASYNC_LOADER = Key.create<AsyncEditorLoader>("ASYNC_LOADER")
-
 class AsyncEditorLoader internal constructor(private val project: Project,
                                             private val provider: TextEditorProvider,
-                                             @JvmField val coroutineScope: CoroutineScope) {
-  private val delayedActions = ArrayDeque<Runnable>()
+                                             @JvmField val coroutineScope: CoroutineScope,
+                                             editor: EditorImpl,
+                                             virtualFile: VirtualFile, task: Deferred<Unit>?) {
+  private val tasks: List<Deferred<Unit>>
+  init {
+    val textEditorInit = coroutineScope.async(CoroutineName("HighlighterTextEditorInitializer")) {
+      TextEditorImpl.setHighlighterToEditor(project, virtualFile, editor.document, editor)
+    }
+    tasks = if (task == null) listOf(textEditorInit) else listOf(textEditorInit, task)
+  }
+  /**
+   * [delayedActions] contains either:
+   * - empty list: the editor was not loaded
+   * - list of runnables: the editor was not loaded and these runnables need to be run on load
+   * - [LOADED]: the editor is loaded
+   *  empty list was chosen to mark editor "not loaded" as early as possible, to avoid a narrow data race between TextEditorImpl instantiation and [AsyncEditorLoader.start] call
+   */
+  private val LOADED: List<Runnable> = listOf(Runnable {})
+  private val delayedActions: AtomicReference<List<Runnable>> = AtomicReference(listOf())
  private val delayedScrollState = AtomicReference<DelayedScrollState?>()

  companion object {
-    internal val OPENED_IN_BULK = Key.create<Boolean>("EditorSplitters.opened.in.bulk")
+    internal val OPENED_IN_BULK: Key<Boolean> = Key.create("EditorSplitters.opened.in.bulk")

    @Internal
    fun isOpenedInBulk(file: VirtualFile): Boolean = file.getUserData(OPENED_IN_BULK) != null

+    private fun findTextEditor(editor: Editor): TextEditor? {
+      val project = editor.project
+      val virtualFile = editor.virtualFile
+      if (project == null || virtualFile == null) return null
+      return FileEditorManager.getInstance(project).getAllEditors(virtualFile).find { f -> f is TextEditor } as TextEditor?
+    }
+
    @JvmStatic
    @RequiresEdt
    fun performWhenLoaded(editor: Editor, runnable: Runnable) {
-      val loader = editor.getUserData(ASYNC_LOADER)
-      loader?.delayedActions?.add(captureThreadContext(runnable)) ?: runnable.run()
+      val asyncLoader = (findTextEditor(editor) as? TextEditorImpl)?.asyncLoader
+      if (asyncLoader == null) {
+        runnable.run()
+      }
+      else {
+        asyncLoader.performWhenLoaded(runnable)
+      }
    }

    internal suspend fun waitForLoaded(editor: Editor) {
-      if (editor.getUserData(ASYNC_LOADER) != null) {
+      if (!isEditorLoaded(editor)) {
        withContext(Dispatchers.EDT + ModalityState.any().asContextElement()) {
          suspendCancellableCoroutine {
            performWhenLoaded(editor) { it.resume(Unit) }
@@ -61,19 +90,32 @@ class AsyncEditorLoader internal constructor(private val project: Project,

    @JvmStatic
    fun isEditorLoaded(editor: Editor): Boolean {
-      return editor.getUserData(ASYNC_LOADER) == null
+      val textEditor = findTextEditor(editor)
+      if (textEditor !is TextEditorImpl) return true
+      return textEditor.isLoaded()
+    }
+  }
+
+  @RequiresEdt
+  private fun performWhenLoaded(runnable: Runnable) {
+    val toRunLater = captureThreadContext(runnable)
+    val newActions = delayedActions.updateAndGet { oldActions: List<Runnable> ->
+      if (oldActions == LOADED || oldActions.contains(toRunLater)) oldActions
+      else oldActions + toRunLater
+    }
+    if (!newActions.contains(toRunLater)) {
+      runnable.run()
    }
  }

  // executed in the same EDT task where TextEditorImpl is created
  @Internal
  @RequiresEdt
-  fun start(textEditor: TextEditorImpl, tasks: List<Deferred<*>>) {
+  fun start(textEditor: TextEditorImpl) {
    val editor = textEditor.editor
-    editor.putUserData(ASYNC_LOADER, this)

    if (ApplicationManager.getApplication().isUnitTestMode) {
-      startInTests(tasks = tasks, editor = editor)
+      startInTests(tasks = tasks)
      return
    }

@@ -89,12 +131,9 @@ class AsyncEditorLoader internal constructor(private val project: Project,
      indicatorJob.cancel()

      withContext(Dispatchers.EDT + CoroutineName("execute delayed actions")) {
-        editor.putUserData(ASYNC_LOADER, null)
        editor.scrollingModel.disableAnimation()
        try {
-          while (true) {
-            (delayedActions.pollFirst() ?: break).run()
-          }
+          markLoadedAndExecuteDelayedActions()
        }
        finally {
          editor.scrollingModel.enableAnimation()
@@ -104,15 +143,18 @@ class AsyncEditorLoader internal constructor(private val project: Project,
    }
  }

-  private fun startInTests(tasks: List<Deferred<*>>, editor: EditorEx) {
+  private fun markLoadedAndExecuteDelayedActions() {
+    val delayedActions = delayedActions.getAndSet(LOADED)
+    for (action in delayedActions) {
+      action.run()
+    }
+  }
+
+  private fun startInTests(tasks: List<Deferred<*>>) {
    runWithModalProgressBlocking(project, "") {
      tasks.awaitAll()
    }
-    editor.putUserData(ASYNC_LOADER, null)
-
-    while (true) {
-      (delayedActions.pollFirst() ?: break).run()
-    }
+    markLoadedAndExecuteDelayedActions()
  }

  @RequiresReadLock
@@ -136,6 +178,10 @@ class AsyncEditorLoader internal constructor(private val project: Project,
  internal fun dispose() {
    coroutineScope.cancel()
  }
+
+  internal fun isLoaded(): Boolean {
+    return delayedActions.get() == LOADED
+  }
 }

 private class DelayedScrollState(@JvmField val relativeCaretPosition: Int, @JvmField val exactState: Boolean)
@@ -33,10 +33,9 @@ import com.intellij.platform.diagnostic.telemetry.impl.span
 import com.intellij.platform.util.coroutines.childScope
 import com.intellij.pom.Navigatable
 import com.intellij.psi.PsiDocumentManager
-import kotlinx.coroutines.CoroutineName
 import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.Deferred
 import kotlinx.coroutines.Dispatchers
-import kotlinx.coroutines.async
 import org.jetbrains.annotations.ApiStatus.Internal
 import org.jetbrains.annotations.NonNls
 import java.beans.PropertyChangeListener
@@ -46,33 +45,20 @@ import kotlin.coroutines.EmptyCoroutineContext

 private val TRANSIENT_EDITOR_STATE_KEY = Key.create<TransientEditorState>("transientState")

-open class TextEditorImpl @Internal constructor(@JvmField protected val project: Project,
-                                                @JvmField protected val file: VirtualFile,
-                                                editor: EditorImpl,
-                                                private val asyncLoader: AsyncEditorLoader) : UserDataHolderBase(), TextEditor {
+open class TextEditorImpl
+@Internal @JvmOverloads constructor(@JvmField protected val project: Project,
+                                    @JvmField protected val file: VirtualFile,
+                                    provider: TextEditorProvider,
+                                    editor: EditorImpl,
+                                    internal val asyncLoader: AsyncEditorLoader = createAsyncEditorLoader(provider, project, editor, file)) : UserDataHolderBase(), TextEditor {
  @Suppress("LeakingThis")
  private val changeSupport = PropertyChangeSupport(this)
  private val component: TextEditorComponent

-  constructor(project: Project,
-              file: VirtualFile,
-              provider: TextEditorProvider,
-              editor: EditorImpl) : this(project = project,
-                                         file = file,
-                                         editor = editor,
-                                         asyncLoader = createAsyncEditorLoader(provider, project)) {
-    val editorSupplier = suspend { editor }
-    @Suppress("LeakingThis")
-    asyncLoader.start(textEditor = this, tasks = listOf(
-      asyncLoader.coroutineScope.async(CoroutineName("HighlighterTextEditorInitializer")) {
-        setHighlighterToEditor(project, file, editor.document, editorSupplier)
-      },
-    ))
-  }
-
  init {
    @Suppress("LeakingThis")
    component = createEditorComponent(project = project, file = file, editor = editor)
+    asyncLoader.start(this)
    for (customizer in TextEditorCustomizer.EP.extensionList) {
      @Suppress("LeakingThis")
      customizer.customize(this)
@@ -89,18 +75,25 @@ open class TextEditorImpl @Internal constructor(@JvmField protected val project:

  // don't pollute global scope
  companion object {
-    fun createAsyncEditorLoader(provider: TextEditorProvider, project: Project): AsyncEditorLoader {
+    fun createAsyncEditorLoader(provider: TextEditorProvider, project: Project, editor: EditorImpl, virtualFile: VirtualFile, task: Deferred<Unit>? = null): AsyncEditorLoader {
+      return AsyncEditorLoader(
+        project = project,
+        provider = provider,
+        coroutineScope = editorLoaderScope(project),
+        editor, virtualFile, task
+      )
+    }
+
+    @Internal
+    fun editorLoaderScope(project: Project): CoroutineScope {
      // `openEditorImpl` uses runWithModalProgressBlocking,
      // but an async editor load is performed in the background, out of the `openEditorImpl` call
      val modality = ModalityState.any().asContextElement()

      val context = if (StartUpMeasurer.isEnabled()) rootTask() else EmptyCoroutineContext
-      return AsyncEditorLoader(
-        project = project,
-        provider = provider,
-        coroutineScope = project.service<AsyncEditorLoaderService>().coroutineScope.childScope(supervisor = false,
-                                                                                               context = context + modality),
-      )
+      val coroutineScope = project.service<AsyncEditorLoaderService>().coroutineScope.childScope(supervisor = false,
+                                                                                                 context = context + modality)
+      return coroutineScope
    }

    @Internal
@@ -122,10 +115,10 @@ open class TextEditorImpl @Internal constructor(@JvmField protected val project:
      return factory.createMainEditor(document!!, project, file, null)
    }

-    private suspend fun setHighlighterToEditor(project: Project,
-                                       file: VirtualFile,
-                                       document: Document,
-                                       editorSupplier: suspend () -> EditorEx) {
+    internal suspend fun setHighlighterToEditor(project: Project,
+                                                file: VirtualFile,
+                                                document: Document,
+                                                editor: EditorImpl) {
      val scheme = serviceAsync<EditorColorsManager>().globalScheme
      val editorHighlighterFactory = serviceAsync<EditorHighlighterFactory>()
      val highlighter = readAction {
@@ -136,7 +129,6 @@ open class TextEditorImpl @Internal constructor(@JvmField protected val project:
        highlighter
      }

-      val editor = editorSupplier()
      span("editor highlighter set", Dispatchers.EDT) {
        editor.settings.setLanguageSupplier { getDocumentLanguage(editor) }
        editor.highlighter = highlighter
@@ -215,6 +207,10 @@ open class TextEditorImpl @Internal constructor(@JvmField protected val project:
  }

  override fun toString(): @NonNls String = "Editor: ${component.file}"
+
+  internal fun isLoaded(): Boolean {
+    return asyncLoader.isLoaded()
+  }
 }

 private class TransientEditorState {
@@ -17,6 +17,7 @@ import com.intellij.openapi.editor.impl.view.FontLayoutService;
 import com.intellij.openapi.editor.markup.HighlighterTargetArea;
 import com.intellij.openapi.editor.markup.RangeHighlighter;
 import com.intellij.openapi.editor.markup.TextAttributes;
+import com.intellij.openapi.fileEditor.impl.text.AsyncEditorLoader;
 import com.intellij.openapi.ide.CopyPasteManager;
 import com.intellij.openapi.util.Ref;
 import com.intellij.openapi.util.SystemInfo;
@@ -431,6 +432,7 @@ public class EditorImplTest extends AbstractEditorTest {

  public void testDefaultHorizontalScrolling() {
    initText("<caret>" + StringUtil.repeat("abc", 100));
+    assertTrue(AsyncEditorLoader.Companion.isEditorLoaded(getEditor()));
    int spaceWidth = EditorUtil.getSpaceWidth(Font.PLAIN, getEditor());
    EditorTestUtil.setEditorVisibleSize(getEditor(), 15, 2);

@@ -0,0 +1,61 @@
+// Copyright 2000-2024 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
+package com.intellij.openapi.editor.impl;
+
+import com.intellij.diagnostic.ThreadDumper;
+import com.intellij.openapi.application.ApplicationManager;
+import com.intellij.openapi.editor.Editor;
+import com.intellij.openapi.editor.EditorFactory;
+import com.intellij.openapi.editor.event.EditorFactoryEvent;
+import com.intellij.openapi.editor.event.EditorFactoryListener;
+import com.intellij.openapi.fileEditor.FileEditor;
+import com.intellij.openapi.fileEditor.OpenFileDescriptor;
+import com.intellij.openapi.fileEditor.impl.text.AsyncEditorLoader;
+import com.intellij.openapi.vfs.VirtualFile;
+import com.intellij.testFramework.FileEditorManagerTestCase;
+import org.jetbrains.annotations.NotNull;
+
+import java.nio.charset.StandardCharsets;
+import java.util.List;
+import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
+
+public class EditorLoadTest extends FileEditorManagerTestCase {
+  public void testEditorLoadedStateMustTransitionInOneDirection_Stress() throws Exception {
+    VirtualFile virtualFile = createTempFile(getTestName(false) + ".txt", "text".getBytes(StandardCharsets.UTF_8));
+    AtomicReference<Future<?>> testStateInvariant = new AtomicReference<>();
+    AtomicBoolean run = new AtomicBoolean();
+    AtomicBoolean loaded = new AtomicBoolean();
+    EditorFactory.getInstance().addEditorFactoryListener(new EditorFactoryListener() {
+      @Override
+      public void editorCreated(@NotNull EditorFactoryEvent event) {
+        Editor editor = event.getEditor();
+        testStateInvariant.set(ApplicationManager.getApplication().executeOnPooledThread(() -> {
+          while (run.get()) {
+            boolean markedLoaded = AsyncEditorLoader.Companion.isEditorLoaded(editor);
+            boolean oldLoaded = loaded.getAndSet(markedLoaded);
+            if (oldLoaded && !markedLoaded) {
+              throw new AssertionError("incorrect state transition: loaded->!loaded\nthreaddump:\n" + ThreadDumper.dumpThreadsToString());
+            }
+          }
+        }));
+      }
+    }, getTestRootDisposable());
+    for (int i=0; i<100; i++) {
+      testStateInvariant.set(null);
+      run.set(true);
+      loaded.set(false);
+      List<FileEditor> editors = manager.openEditor(new OpenFileDescriptor(getProject(), virtualFile, 0), false);
+      LOG.debug(i+": "+editors);
+      System.out.println(i+": "+editors);
+      try {
+        assertTrue(loaded.get());
+        run.set(false);
+        testStateInvariant.get().get();
+      }
+      finally {
+        manager.closeFile(virtualFile);
+      }
+    }
+  }
+}