diff --git a/platform/core-api/src/com/intellij/openapi/project/DumbService.java b/platform/core-api/src/com/intellij/openapi/project/DumbService.java index c010e639ac49..98a197742d35 100644 --- a/platform/core-api/src/com/intellij/openapi/project/DumbService.java +++ b/platform/core-api/src/com/intellij/openapi/project/DumbService.java @@ -248,6 +248,8 @@ public abstract class DumbService { * (which could start "dumb mode") some reference resolve is required (which again requires "smart mode").

*

* 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(); diff --git a/platform/lang-impl/src/com/intellij/util/indexing/FileBasedIndexProjectHandler.java b/platform/lang-impl/src/com/intellij/util/indexing/FileBasedIndexProjectHandler.java index 8e0f96bb20e9..f0665c858e5b 100644 --- a/platform/lang-impl/src/com/intellij/util/indexing/FileBasedIndexProjectHandler.java +++ b/platform/lang-impl/src/com/intellij/util/indexing/FileBasedIndexProjectHandler.java @@ -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 files, - final Project project, - final FileBasedIndexImpl index) { + @VisibleForTesting + @ApiStatus.Internal + public static void reindexRefreshedFiles(ProgressIndicator indicator, Collection files, Project project) { + FileBasedIndexImpl index = (FileBasedIndexImpl)FileBasedIndex.getInstance(); CacheUpdateRunner.processFiles(indicator, files, project, content -> index.processRefreshedFile(project, content)); } } diff --git a/platform/platform-impl/src/com/intellij/openapi/application/impl/ApplicationImpl.java b/platform/platform-impl/src/com/intellij/openapi/application/impl/ApplicationImpl.java index 2b38a21fd6be..63e63ca0a503 100644 --- a/platform/platform-impl/src/com/intellij/openapi/application/impl/ApplicationImpl.java +++ b/platform/platform-impl/src/com/intellij/openapi/application/impl/ApplicationImpl.java @@ -226,7 +226,6 @@ public class ApplicationImpl extends PlatformComponentManagerImpl implements App @NotNull @Override public Future executeOnPooledThread(@SuppressWarnings("BoundedWildcard") @NotNull Callable action) { - ReadMostlyRWLock.SuspensionId suspensionId = myLock.currentReadPrivilege(); return ourThreadExecutorsService.submit(new Callable() { @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; } diff --git a/platform/platform-impl/src/com/intellij/openapi/application/impl/ReadMostlyRWLock.java b/platform/platform-impl/src/com/intellij/openapi/application/impl/ReadMostlyRWLock.java index 986778a629bc..2a8f5efe2c93 100644 --- a/platform/platform-impl/src/com/intellij/openapi/application/impl/ReadMostlyRWLock.java +++ b/platform/platform-impl/src/com/intellij/openapi/application/impl/ReadMostlyRWLock.java @@ -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 readers = ContainerUtil.createConcurrentList(); - private final Map 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 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 + '}'; } } diff --git a/platform/platform-impl/src/com/intellij/openapi/project/CacheUpdateRunner.java b/platform/platform-impl/src/com/intellij/openapi/project/CacheUpdateRunner.java index 15ca31700594..e369b2d44bf6 100644 --- a/platform/platform-impl/src/com/intellij/openapi/project/CacheUpdateRunner.java +++ b/platform/platform-impl/src/com/intellij/openapi/project/CacheUpdateRunner.java @@ -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 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); diff --git a/platform/platform-tests/testSrc/com/intellij/openapi/application/impl/ApplicationImplTest.java b/platform/platform-tests/testSrc/com/intellij/openapi/application/impl/ApplicationImplTest.java index f842df17f7a3..88bb1c5d61fb 100644 --- a/platform/platform-tests/testSrc/com/intellij/openapi/application/impl/ApplicationImplTest.java +++ b/platform/platform-tests/testSrc/com/intellij/openapi/application/impl/ApplicationImplTest.java @@ -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> futures = new ArrayList<>(); - - ApplicationImpl app = (ApplicationImpl)ApplicationManager.getApplication(); - List 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 = 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 = 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(); diff --git a/platform/platform-tests/testSrc/com/intellij/openapi/project/DumbServiceImplTest.groovy b/platform/platform-tests/testSrc/com/intellij/openapi/project/DumbServiceImplTest.groovy index 031f268bdcc8..d9d26c860323 100644 --- a/platform/platform-tests/testSrc/com/intellij/openapi/project/DumbServiceImplTest.groovy +++ b/platform/platform-tests/testSrc/com/intellij/openapi/project/DumbServiceImplTest.groovy @@ -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() + } }