remove app read lock privileges

because they're complicated and produce deadlocks during modal indexing when FileContentQueue needs encoding which needs a read action (e.g. JSP) GitOrigin-RevId: b85fbeea47f724fdc8cd8333d9a87e33ac7ceeaf
2026-09-27 10:03:11 +07:00 · 2019-12-04 09:08:56 +00:00
parent f21bbb94a3
commit f0d1d64f66
7 changed files with 85 additions and 193 deletions
@@ -248,6 +248,8 @@ public abstract class DumbService {
   * (which could start "dumb mode") some reference resolve is required (which again requires "smart mode").<p/>
   * <p>
   * Should be invoked on dispatch thread.
+   * It's the caller's responsibility to invoke this method only when the model is in internally consistent state,
+   * so that background threads with read actions don't see half-baked PSI/VFS/etc.
   */
  public abstract void completeJustSubmittedTasks();

@@ -5,6 +5,7 @@
 */
 package com.intellij.util.indexing;

+import com.google.common.annotations.VisibleForTesting;
 import com.intellij.diagnostic.PerformanceWatcher;
 import com.intellij.ide.IdeBundle;
 import com.intellij.openapi.Disposable;
@@ -129,7 +130,7 @@ public final class FileBasedIndexProjectHandler implements IndexableFileSet, Dis
        LOG.info("Reindexing refreshed files: " + files.size() + " to update, calculated in " + calcDuration + "ms");
        if (!files.isEmpty()) {
          PerformanceWatcher.Snapshot snapshot = PerformanceWatcher.takeSnapshot();
-          reindexRefreshedFiles(indicator, files, project, index);
+          reindexRefreshedFiles(indicator, files, project);
          snapshot.logResponsivenessSinceCreation("Reindexing refreshed files");
        }
      }
@@ -177,10 +178,10 @@ public final class FileBasedIndexProjectHandler implements IndexableFileSet, Dis
    });
  }

-  private static void reindexRefreshedFiles(ProgressIndicator indicator,
-                                            Collection<VirtualFile> files,
-                                            final Project project,
-                                            final FileBasedIndexImpl index) {
+  @VisibleForTesting
+  @ApiStatus.Internal
+  public static void reindexRefreshedFiles(ProgressIndicator indicator, Collection<VirtualFile> files, Project project) {
+    FileBasedIndexImpl index = (FileBasedIndexImpl)FileBasedIndex.getInstance();
    CacheUpdateRunner.processFiles(indicator, files, project, content -> index.processRefreshedFile(project, content));
  }
 }
@@ -226,7 +226,6 @@ public class ApplicationImpl extends PlatformComponentManagerImpl implements App
  @NotNull
  @Override
  public <T> Future<T> executeOnPooledThread(@SuppressWarnings("BoundedWildcard") @NotNull Callable<T> action) {
-    ReadMostlyRWLock.SuspensionId suspensionId = myLock.currentReadPrivilege();
    return ourThreadExecutorsService.submit(new Callable<T>() {
      @Override
      public T call() {
@@ -234,16 +233,7 @@ public class ApplicationImpl extends PlatformComponentManagerImpl implements App
          return null;
        }

-        // This is very special magic only needed by threads that need read actions and can be executed
-        // during "executeSuspendingWriteAction" (e.g. dumb mode, indexing). Threads created via "executeOnPooledThread"
-        // in these circumstances may run read actions immediately, instead of waiting until the write action is resumed and finished.
-
-        // For everyone else, "executeOnPooledThread" should be equivalent to "AppExecutorUtil" AKA "PooledThreadExecutor" pool
-        try (AccessToken ignored = myLock.applyReadPrivilege(suspensionId)) {
-          if (isDisposed()) {
-            return null;
-          }
-
+        try {
          return action.call();
        }
        catch (ProcessCanceledException e) {
@@ -1175,6 +1165,13 @@ public class ApplicationImpl extends PlatformComponentManagerImpl implements App
    return myLock.isWriteLocked();
  }

+  /**
+   * If called inside a write action, executes the given code under a modal progress with write lock released (e.g. to allow for read-action parallelization).
+   * It's the caller's responsibility to invoke this method only when the model is in internally consistent state,
+   * so that background threads with read actions don't see half-baked PSI/VFS/etc. The runnable may perform write actions itself,
+   * callers should be ready for those.
+   */
+  @ApiStatus.Internal
  public void executeSuspendingWriteAction(@Nullable Project project, @NotNull String title, @NotNull Runnable runnable) {
    assertIsDispatchThread();
    if (!myLock.isWriteLocked()) {
@@ -1185,11 +1182,7 @@ public class ApplicationImpl extends PlatformComponentManagerImpl implements App
    int prevBase = myWriteStackBase;
    myWriteStackBase = myWriteActionsStack.size();
    try (AccessToken ignored = myLock.writeSuspend()) {
-      runModalProgress(project, title, () -> {
-        try (AccessToken ignored1 = myLock.grantReadPrivilege()) {
-          runnable.run();
-        }
-      });
+      runModalProgress(project, title, runnable);
    } finally {
      myWriteStackBase = prevBase;
    }
@@ -15,22 +15,16 @@
 */
 package com.intellij.openapi.application.impl;

-import com.intellij.diagnostic.ThreadDumper;
 import com.intellij.openapi.application.AccessToken;
 import com.intellij.openapi.application.ex.ApplicationUtil;
-import com.intellij.openapi.diagnostic.Attachment;
-import com.intellij.openapi.diagnostic.Logger;
 import com.intellij.openapi.progress.ProgressManager;
 import com.intellij.openapi.progress.impl.CoreProgressManager;
 import com.intellij.util.containers.ConcurrentList;
 import com.intellij.util.containers.ContainerUtil;
 import org.jetbrains.annotations.NotNull;
-import org.jetbrains.annotations.Nullable;

 import java.util.ArrayList;
 import java.util.List;
-import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.locks.LockSupport;

 /**
@@ -47,16 +41,13 @@ import java.util.concurrent.locks.LockSupport;
 * Write lock: sets global {@link #writeRequested} bit and waits for all readers (in global {@link #readers} list) to release their locks by checking {@link Reader#readRequested} for all readers.
 */
 class ReadMostlyRWLock {
-  private static final Logger LOG = Logger.getInstance(ReadMostlyRWLock.class);
  private final Thread writeThread;
  volatile boolean writeRequested;  // this writer is requesting or obtained the write access
  private volatile boolean writeAcquired;   // this writer obtained the write lock
  // All reader threads are registered here. Dead readers are garbage collected in writeUnlock().
  private final ConcurrentList<Reader> readers = ContainerUtil.createConcurrentList();

-  private final Map<Thread, SuspensionId> privilegedReaders = new ConcurrentHashMap<>();
-
-  private volatile SuspensionId currentSuspension;
+  private volatile boolean writeSuspended;

  ReadMostlyRWLock(@NotNull Thread writeThread) {
    this.writeThread = writeThread;
@@ -112,11 +103,11 @@ class ReadMostlyRWLock {
    Reader status = R.get();
    throwIfImpatient(status);

-    if (tryReadLock(status, true)) {
+    if (tryReadLock(status)) {
      return;
    }
    for (int iter = 0; ; iter++) {
-      if (tryReadLock(status, true)) {
+      if (tryReadLock(status)) {
        break;
      }

@@ -183,15 +174,12 @@ class ReadMostlyRWLock {
  boolean tryReadLock() {
    checkReadThreadAccess();
    Reader status = R.get();
-    return tryReadLock(status, true);
+    return tryReadLock(status);
  }

-  private boolean tryReadLock(Reader status, boolean checkPrivileges) {
+  private boolean tryReadLock(Reader status) {
    throwIfImpatient(status);
    if (!writeRequested) {
-      if (checkPrivileges && currentSuspension != null && !privilegedReaders.containsKey(Thread.currentThread())) {
-        return false;
-      }
      status.readRequested = true;
      if (!writeRequested) {
        return true;
@@ -225,63 +213,14 @@ class ReadMostlyRWLock {
  }

  AccessToken writeSuspend() {
-    SuspensionId prevSuspension = currentSuspension;
-    if (prevSuspension == null) {
-      currentSuspension = new SuspensionId();
-    }
+    boolean prev = writeSuspended;
+    writeSuspended = true;
    writeUnlock();
    return new AccessToken() {
      @Override
      public void finish() {
        writeLock();
-        currentSuspension = prevSuspension;
-        if (prevSuspension == null) {
-          ensureNoPrivilegedReaders();
-        }
-      }
-    };
-  }
-
-  private void ensureNoPrivilegedReaders() {
-    if (!privilegedReaders.isEmpty()) {
-      List<String> offenderNames = ContainerUtil.map(privilegedReaders.keySet(), Thread::getName);
-      privilegedReaders.clear();
-      LOG.error("Pooled threads created during write action suspension should have been terminated: " + offenderNames,
-                new Attachment("threadDump.txt", ThreadDumper.dumpThreadsToString()));
-    }
-  }
-
-  @Nullable
-  SuspensionId currentReadPrivilege() {
-    return privilegedReaders.get(Thread.currentThread());
-  }
-
-  @NotNull AccessToken applyReadPrivilege(@Nullable SuspensionId context) {
-    Reader status = R.get();
-    int iter = 0;
-    while (context != null && context == currentSuspension) {
-      if (tryReadLock(status, false)) {
-        try {
-          return context == currentSuspension ? grantReadPrivilege() : AccessToken.EMPTY_ACCESS_TOKEN;
-        }
-        finally {
-          readUnlock();
-        }
-      }
-
-      waitABit(status, iter++);
-    }
-    return AccessToken.EMPTY_ACCESS_TOKEN;
-  }
-
-  @NotNull
-  AccessToken grantReadPrivilege() {
-    Thread thread = Thread.currentThread();
-    privilegedReaders.put(thread, currentSuspension);
-    return new AccessToken() {
-      @Override
-      public void finish() {
-        privilegedReaders.remove(thread);
+        writeSuspended = prev;
      }
    };
  }
@@ -328,8 +267,6 @@ class ReadMostlyRWLock {
    return writeAcquired;
  }

-  static class SuspensionId {}
-
  @Override
  public String toString() {
    return "ReadMostlyRWLock{" +
@@ -337,8 +274,7 @@ class ReadMostlyRWLock {
           ", writeRequested=" + writeRequested +
           ", writeAcquired=" + writeAcquired +
           ", readers=" + readers +
-           ", privilegedReaders=" + privilegedReaders +
-           ", currentSuspension=" + currentSuspension +
+           ", writeSuspended=" + writeSuspended +
           '}';
  }
 }
@@ -14,6 +14,7 @@ import com.intellij.openapi.progress.ProcessCanceledException;
 import com.intellij.openapi.progress.ProgressIndicator;
 import com.intellij.openapi.progress.ProgressManager;
 import com.intellij.openapi.progress.util.ProgressIndicatorBase;
+import com.intellij.openapi.progress.util.ProgressIndicatorUtils;
 import com.intellij.openapi.progress.util.ProgressWrapper;
 import com.intellij.openapi.util.Disposer;
 import com.intellij.openapi.util.Key;
@@ -45,6 +46,8 @@ public class CacheUpdateRunner {
    final double total = files.size();
    queue.startLoading();

+    indicator.setIndeterminate(false);
+
    ProgressUpdater progressUpdater = new ProgressUpdater() {
      final Set<VirtualFile> myFilesBeingProcessed = new THashSet<>();
      final AtomicInteger myNumberOfFilesProcessed = new AtomicInteger();
@@ -163,7 +166,7 @@ public class CacheUpdateRunner {
    assert !ApplicationManager.getApplication().isWriteAccessAllowed();
    try {
      for (Future<?> future : futures) {
-        future.get();
+        ProgressIndicatorUtils.awaitWithCheckCanceled(future);
      }

      boolean allFinished = true;
@@ -175,7 +178,8 @@ public class CacheUpdateRunner {
      }
      return allFinished;
    }
-    catch (InterruptedException ignored) {
+    catch (ProcessCanceledException e) {
+      throw e;
    }
    catch (Throwable throwable) {
      LOG.error(throwable);
@@ -585,39 +585,6 @@ public class ApplicationImplTest extends LightPlatformTestCase {
    if (e.get() != null) throw e.get();
  }

-  public void testSuspendWriteActionDelaysForeignReadActions() throws Throwable {
-    Semaphore mayStartForeignRead = new Semaphore();
-    mayStartForeignRead.down();
-
-    List<Future<?>> futures = new ArrayList<>();
-
-    ApplicationImpl app = (ApplicationImpl)ApplicationManager.getApplication();
-    List<String> log = Collections.synchronizedList(new ArrayList<>());
-    futures.add(app.executeOnPooledThread(() -> {
-      assertTrue(mayStartForeignRead.waitFor(1000));
-      ReadAction.run(() -> log.add("foreign read"));
-    }));
-
-    safeWrite(() -> {
-      log.add("write started");
-      app.executeSuspendingWriteAction(getProject(), "", () -> {
-        app.invokeAndWait(() ->
-          futures.add(app.executeOnPooledThread(() -> ReadAction.run(() -> log.add("foreign read")))));
-
-        mayStartForeignRead.up();
-        TimeoutUtil.sleep(50);
-
-        ReadAction.run(() -> log.add("progress read"));
-        app.invokeAndWait(() -> WriteAction.run(() -> log.add("nested write")));
-        waitForFuture(app.executeOnPooledThread(() -> ReadAction.run(() -> log.add("forked read"))));
-      });
-      log.add("write finished");
-    });
-
-    futures.forEach(ApplicationImplTest::waitForFuture);
-    assertOrderedEquals(log, "write started", "progress read", "nested write", "forked read", "write finished", "foreign read", "foreign read");
-  }
-
  private static void waitForFuture(Future<?> future) {
    try {
      future.get(10_000, TimeUnit.MILLISECONDS);
@@ -645,70 +612,6 @@ public class ApplicationImplTest extends LightPlatformTestCase {
    safeWrite(runnable);
  }

-  public void testPooledThreadsThatHappenInSuspendedWriteActionStayInSuspendedWriteAction() throws Throwable {
-    LoggedErrorProcessor.getInstance().disableStderrDumping(getTestRootDisposable());
-
-    Ref<Future<?>> future = Ref.create();
-    ApplicationImpl app = (ApplicationImpl)ApplicationManager.getApplication();
-    safeWrite(() -> {
-      try {
-        Semaphore started = new Semaphore();
-        started.down();
-        app.executeSuspendingWriteAction(getProject(), "", () -> {
-          future.set(app.executeOnPooledThread(() -> {
-            started.up();
-            TimeoutUtil.sleep(1000);
-          }));
-          assertTrue(started.waitFor(1000));
-        });
-        fail("should not allow pooled thread to stay there");
-      }
-      catch (AssertionError e) {
-        assertTrue(ExceptionUtil.getThrowableText(e), isEscapingThreadAssertion(e));
-      }
-    });
-    waitForFuture(future.get());
-  }
-
-  public void testPooledThreadsStartedAfterQuickSuspendedWriteActionDontGetReadPrivileges() throws Throwable {
-    for (int i = 0; i < 1000; i++) {
-      safeWrite(this::checkPooledThreadsDontGetWrongPrivileges);
-    }
-  }
-
-  private void checkPooledThreadsDontGetWrongPrivileges() {
-    ApplicationImpl app = (ApplicationImpl)ApplicationManager.getApplication();
-    Ref<Future<?>> future = Ref.create();
-
-    Disposable disableStderrDumping = Disposer.newDisposable();
-    LoggedErrorProcessor.getInstance().disableStderrDumping(disableStderrDumping);
-
-    Semaphore mayFinish = new Semaphore();
-    mayFinish.down();
-    try {
-      app.executeSuspendingWriteAction(getProject(), "", () ->
-        future.set(app.executeOnPooledThread(
-          () -> assertTrue(mayFinish.waitFor(5_000)))));
-    }
-    catch (AssertionError e) {
-      if (!isEscapingThreadAssertion(e)) {
-        e.printStackTrace();
-        throw e;
-      }
-    }
-    finally {
-      Disposer.dispose(disableStderrDumping);
-    }
-
-    app.executeSuspendingWriteAction(getProject(), "", () -> {});
-    mayFinish.up();
-    waitForFuture(future.get());
-  }
-
-  private static boolean isEscapingThreadAssertion(AssertionError e) {
-    return e.getMessage().contains("should have been terminated");
-  }
-
  public void testReadActionInImpatientModeShouldThrowWhenThereIsAPendingWrite() throws Throwable {
    AtomicBoolean stopRead = new AtomicBoolean();
    AtomicBoolean readAcquired = new AtomicBoolean();
@@ -16,12 +16,23 @@
 package com.intellij.openapi.project

 import com.intellij.openapi.application.ApplicationManager
+import com.intellij.openapi.application.WriteAction
+import com.intellij.openapi.progress.ProcessCanceledException
 import com.intellij.openapi.progress.ProgressIndicator
+import com.intellij.openapi.progress.util.ProgressIndicatorUtils
+import com.intellij.openapi.vfs.LocalFileSystem
+import com.intellij.openapi.vfs.newvfs.impl.VirtualFileImpl
+import com.intellij.psi.impl.PsiManagerImpl
 import com.intellij.testFramework.fixtures.BasePlatformTestCase
+import com.intellij.testFramework.fixtures.impl.TempDirTestFixtureImpl
 import com.intellij.util.TimeoutUtil
 import com.intellij.util.concurrency.Semaphore
+import com.intellij.util.indexing.FileBasedIndexProjectHandler
 import com.intellij.util.ui.UIUtil
 import org.jetbrains.annotations.NotNull
+
+import java.util.concurrent.atomic.AtomicBoolean
+
 /**
 * @author peter
 */
@@ -64,4 +75,46 @@ class DumbServiceImplTest extends BasePlatformTestCase {
  private DumbServiceImpl getDumbService() {
    (DumbServiceImpl)DumbService.getInstance(project)
  }
+
+  void "test no deadlocks when indexing JSP modally"() {
+    def tempFixture = new TempDirTestFixtureImpl()
+    disposeOnTearDown { tempFixture.tearDown() }
+
+    // create externally and carefully refresh, avoiding eager content loading and charset detection
+    def dir = new File(tempFixture.tempDirPath + '/jsps')
+    dir.mkdirs()
+    new File(dir, 'a.jsp').createNewFile()
+
+    def vDir = LocalFileSystem.instance.refreshAndFindFileByIoFile(dir)
+    assert vDir != null
+    assert vDir.children.length == 1
+    def child = vDir.children[0]
+    assert child.fileType.name == 'JSP'
+    assert !((VirtualFileImpl) child).charsetSet
+    assert ((PsiManagerImpl)psiManager).fileManager.getCachedPsiFile(child) == null
+
+    def started = new AtomicBoolean()
+    def finished = new AtomicBoolean()
+
+    dumbService.queueAsynchronousTask(new DumbModeTask() {
+      @Override
+      void performInDumbMode(@NotNull ProgressIndicator indicator) {
+        started.set(true)
+        assert !ApplicationManager.application.dispatchThread
+        try {
+          ProgressIndicatorUtils.withTimeout(20_000) {
+            FileBasedIndexProjectHandler.reindexRefreshedFiles(indicator, [child], project)
+          }
+        }
+        catch (ProcessCanceledException e) {
+          throw new RuntimeException("Successful indexing expected", e)
+        }
+        finished.set(true)
+      }
+    })
+    assert !started.get()
+    WriteAction.run { dumbService.completeJustSubmittedTasks() }
+    assert started.get()
+    assert finished.get()
+  }
 }