From c4a246b6b4db10bdf132e00a8eed040200c40497 Mon Sep 17 00:00:00 2001 From: peter Date: Thu, 20 Oct 2016 09:18:25 +0200 Subject: [PATCH] enforce that threads created during modal indexing don't leave it (IDEA-CR-14502) --- .../application/impl/ApplicationImpl.java | 8 +++-- .../application/impl/ReadMostlyRWLock.java | 29 ++++++++++++++----- .../application/impl/ApplicationImplTest.java | 21 +++++++++++++- 3 files changed, 47 insertions(+), 11 deletions(-) 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 55df2e1274a6..930bfc4b7a3b 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 @@ -1256,8 +1256,12 @@ public class ApplicationImpl extends PlatformComponentManagerImpl implements App try { runModalProgress(project, title, true, runnable); } finally { - myLock.writeResume(); - myWriteStackBase = prevBase; + try { + myLock.writeResume(); + } + 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 2f89ea759475..2ff56fe918f2 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,7 +15,10 @@ */ package com.intellij.openapi.application.impl; +import com.intellij.diagnostic.ThreadDumper; import com.intellij.openapi.application.AccessToken; +import com.intellij.openapi.diagnostic.Attachment; +import com.intellij.openapi.diagnostic.Logger; import com.intellij.openapi.progress.ProgressManager; import com.intellij.util.containers.ConcurrentList; import com.intellij.util.containers.ContainerUtil; @@ -23,6 +26,7 @@ import org.jetbrains.annotations.NotNull; import java.util.ArrayList; import java.util.List; +import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.locks.LockSupport; @@ -40,6 +44,7 @@ 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("#com.intellij.openapi.application.impl.ReadMostlyRWLock"); private final Thread writeThread; private volatile boolean writeRequested; // this writer is requesting or obtained the write access private volatile boolean writeAcquired; // this writer obtained the write lock @@ -47,6 +52,9 @@ class ReadMostlyRWLock { // All reader threads are registered here. Dead readers are garbage collected in writeUnlock(). private final ConcurrentList readers = ContainerUtil.createConcurrentList(); + /** threads that can start read actions during a suspended write write action */ + private final Set privilegedReaders = ContainerUtil.newConcurrentSet(); + ReadMostlyRWLock(@NotNull Thread writeThread) { this.writeThread = writeThread; } @@ -56,8 +64,6 @@ class ReadMostlyRWLock { @NotNull private final Thread thread; // its thread private volatile boolean readRequested; // this reader is requesting or obtained read access. Written by reader thread only, read by writer. private volatile boolean blocked; // this reader is blocked waiting for the writer thread to release write lock. Written by reader thread only, read by writer. - /** >0 when this thread can start read actions during a suspended write write action */ - private int readPrivileges; Reader(@NotNull Thread readerThread) { thread = readerThread; @@ -129,7 +135,7 @@ class ReadMostlyRWLock { private boolean tryReadLock(Reader status) { if (!writeRequested) { - if (writeSuspended.get() > 0 && status.readPrivileges == 0) { + if (writeSuspended.get() > 0 && !isPrivilegedReader()) { return false; } status.readRequested = true; @@ -171,21 +177,28 @@ class ReadMostlyRWLock { void writeResume() { writeLock(); writeSuspended.decrementAndGet(); + + if (!privilegedReaders.isEmpty()) { + List offenderNames = ContainerUtil.map(privilegedReaders, Thread::getName); + privilegedReaders.clear(); + LOG.error("Pooled threads created during write action suspension should have been terminated: " + offenderNames, + new Attachment("threadDump.txt", ThreadDumper.dumpThreadsToString())); + } } boolean isPrivilegedReader() { - return R.get().readPrivileges > 0; + return privilegedReaders.contains(Thread.currentThread()); } AccessToken setupReadPrivilege(boolean allow) { - if (!allow) return AccessToken.EMPTY_ACCESS_TOKEN; + if (!allow || isPrivilegedReader()) return AccessToken.EMPTY_ACCESS_TOKEN; - Reader reader = R.get(); - reader.readPrivileges++; + Thread thread = Thread.currentThread(); + privilegedReaders.add(thread); return new AccessToken() { @Override public void finish() { - reader.readPrivileges--; + privilegedReaders.remove(thread); } }; } 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 73cd989c2ce7..25a9e188c1a1 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 @@ -28,6 +28,7 @@ import com.intellij.openapi.util.Disposer; import com.intellij.openapi.util.EmptyRunnable; import com.intellij.openapi.util.ThrowableComputable; import com.intellij.testFramework.LightPlatformTestCase; +import com.intellij.testFramework.LoggedErrorProcessor; import com.intellij.testFramework.PlatformTestUtil; import com.intellij.util.ArrayUtil; import com.intellij.util.TimeoutUtil; @@ -40,7 +41,9 @@ import javax.swing.*; import java.util.ArrayList; import java.util.Collections; import java.util.List; -import java.util.concurrent.*; +import java.util.concurrent.Callable; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; public class ApplicationImplTest extends LightPlatformTestCase { @@ -601,4 +604,20 @@ public class ApplicationImplTest extends LightPlatformTestCase { })); UIUtil.dispatchAllInvocationEvents(); } + + public void testPooledThreadsThatHappenInSuspendedWriteActionStayInSuspendedWriteAction() { + LoggedErrorProcessor.getInstance().disableStderrDumping(getTestRootDisposable()); + + ApplicationImpl app = (ApplicationImpl)ApplicationManager.getApplication(); + app.invokeLater(() -> WriteAction.run(() -> { + try { + app.executeSuspendingWriteAction(ourProject, "", () -> app.executeOnPooledThread(() -> TimeoutUtil.sleep(1000))); + fail(); + } + catch (AssertionError e) { + assertTrue(e.getMessage().contains("should have been terminated")); + } + })); + UIUtil.dispatchAllInvocationEvents(); + } }