From a276775bb8722785cd551a03819eda26330cfe65 Mon Sep 17 00:00:00 2001 From: Alexey Kudravtsev Date: Tue, 20 Nov 2018 18:41:21 +0300 Subject: [PATCH] fix testReadActionInImpatientModeShouldThrowWhenThereIsAPendingWrite (during tryWrite() writeRequested was reset to false temporarily, which caused discontinuity in tests assuming write lock is in process of acquiring. That reset didn't make sense anyway because all it did was instill false hope to readers waiting in the other threads) --- .../application/impl/ApplicationImpl.java | 2 +- .../application/impl/ReadMostlyRWLock.java | 40 ++++++---- .../application/impl/ApplicationImplTest.java | 79 +++++++++++-------- 3 files changed, 70 insertions(+), 51 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 e9da6d9e2232..e469b42eca97 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 @@ -1221,7 +1221,7 @@ public class ApplicationImpl extends PlatformComponentManagerImpl implements App ActivityTracker.getInstance().inc(); fireBeforeWriteActionStart(clazz); - if (!myLock.isWriteLocked() && !myLock.tryWriteLock()) { + if (!myLock.isWriteLocked()) { Future reportSlowWrite = ourDumpThreadsOnLongWriteActionWaiting <= 0 ? null : JobScheduler.getScheduler() .scheduleWithFixedDelay(() -> PerformanceWatcher.getInstance().dumpThreads("waiting", true), 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 f637cc16f23d..1757cd68eca7 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 @@ -71,6 +71,16 @@ class ReadMostlyRWLock { Reader(@NotNull Thread readerThread) { thread = readerThread; } + + @Override + public String toString() { + return "Reader{" + + "thread=" + thread + + ", readRequested=" + readRequested + + ", blocked=" + blocked + + ", impatientReads=" + impatientReads + + '}'; + } } private final ThreadLocal R = ThreadLocal.withInitial(() -> { @@ -97,7 +107,7 @@ class ReadMostlyRWLock { for (int iter = 0; ; iter++) { if (tryReadLock(status, true)) { - return; + break; } ProgressManager.checkCanceled(); @@ -181,6 +191,7 @@ class ReadMostlyRWLock { } private static final int SPIN_TO_WAIT_FOR_LOCK = 100; + void writeLock() { checkWriteThreadAccess(); assert !writeRequested; @@ -292,21 +303,6 @@ class ReadMostlyRWLock { } } - boolean tryWriteLock() { - checkWriteThreadAccess(); - assert !writeRequested; - assert !writeAcquired; - - writeRequested = true; - if (areAllReadersIdle()) { - writeAcquired = true; - return true; - } - - writeRequested = false; - return false; - } - private boolean areAllReadersIdle() { for (Reader reader : readers) { if (reader.readRequested) { @@ -322,4 +318,16 @@ class ReadMostlyRWLock { } static class SuspensionId {} + + @Override + public String toString() { + return "ReadMostlyRWLock{" + + "writeThread=" + writeThread + + ", writeRequested=" + writeRequested + + ", writeAcquired=" + writeAcquired + + ", readers=" + readers + + ", privilegedReaders=" + privilegedReaders + + ", currentSuspension=" + currentSuspension + + '}'; + } } 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 055287818ad6..18874f650ab6 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 @@ -16,6 +16,7 @@ package com.intellij.openapi.application.impl; import com.intellij.concurrency.JobSchedulerImpl; +import com.intellij.diagnostic.ThreadDumper; import com.intellij.openapi.Disposable; import com.intellij.openapi.application.*; import com.intellij.openapi.application.ex.ApplicationEx; @@ -43,9 +44,9 @@ import java.util.List; import java.util.concurrent.Callable; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; -@SuppressWarnings("StatementWithEmptyBody") @RunFirst public class ApplicationImplTest extends LightPlatformTestCase { @Override @@ -144,24 +145,20 @@ public class ApplicationImplTest extends LightPlatformTestCase { for (int i = 0; i < numOfThreads; i++) { Thread thread = new Thread(() -> { assertFalse(application.isReadAccessAllowed()); - //System.out.println("start "+Thread.currentThread()); for (int i1 = 0; i1 < readIterations; i1++) { application.runReadAction(() -> { }); } - //System.out.println("end "+Thread.currentThread()); }, "read thread " + i); thread.start(); threads.add(thread); } if (writeIterations > 0) { - //System.out.println("write start"); for (int i = 0; i < writeIterations; i++) { ApplicationManager.getApplication().runWriteAction(() -> { }); } - //System.out.println("write end"); } ConcurrencyUtil.joinAll(threads); threads.clear(); @@ -173,10 +170,9 @@ public class ApplicationImplTest extends LightPlatformTestCase { } private long timeOut; - private boolean ok() throws Throwable { + private void checkTimeout() throws Throwable { if (exception != null) throw exception; - if (System.currentTimeMillis() > timeOut) throw new RuntimeException("timeout"); - return true; + if (System.currentTimeMillis() > timeOut) throw new TimeoutException("timeout"); } public void testAppLockReadWritePreference() throws Throwable { @@ -206,7 +202,7 @@ public class ApplicationImplTest extends LightPlatformTestCase { try { LOG.debug("read lock1 acquired"); read1Acquired.set(true); - while (holdRead1.get() && ok()); + while (holdRead1.get()) checkTimeout(); } finally { read1Released.set(true); @@ -221,15 +217,15 @@ public class ApplicationImplTest extends LightPlatformTestCase { }, "read lock1"); readAction1.start(); - while (!read1Acquired.get() && ok()); + while (!read1Acquired.get()) checkTimeout(); AtomicBoolean aboutToAcquireWrite = new AtomicBoolean(); // readActions2 should try to acquire read action when write action is pending Thread readActions2 = new Thread(() -> { try { assertFalse(application.isDispatchThread()); - while (!aboutToAcquireWrite.get() && ok()); + while (!aboutToAcquireWrite.get()) checkTimeout(); // make sure EDT called writelock - while (!application.myLock.writeRequested && ok()); + while (!application.myLock.writeRequested) checkTimeout(); assertTrue(application.isWriteActionPending()); //assertFalse(application.tryRunReadAction(EmptyRunnable.getInstance())); AccessToken stamp = application.acquireReadActionLock(); @@ -254,13 +250,14 @@ public class ApplicationImplTest extends LightPlatformTestCase { Thread checkThread = new Thread(()->{ try { assertFalse(application.isDispatchThread()); - while (!aboutToAcquireWrite.get() && ok()); - while (!read1Acquired.get() && ok()); + while (!aboutToAcquireWrite.get()) checkTimeout(); + while (!read1Acquired.get()) checkTimeout(); // make sure EDT called writelock - while (!application.myLock.writeRequested && ok()); + while (!application.myLock.writeRequested) checkTimeout(); long timeout = System.currentTimeMillis() + 2_000; - while (System.currentTimeMillis() < timeout && ok()) { + while (System.currentTimeMillis() < timeout) { + checkTimeout(); assertTrue(aboutToAcquireWrite.get()); assertTrue(read1Acquired.get()); assertFalse(read1Released.get()); @@ -276,10 +273,11 @@ public class ApplicationImplTest extends LightPlatformTestCase { } holdRead1.set(false); - while (!writeAcquired.get() && ok()); + while (!writeAcquired.get()) checkTimeout(); timeout = System.currentTimeMillis() + 2_000; - while (System.currentTimeMillis() < timeout && ok()) { + while (System.currentTimeMillis() < timeout) { + checkTimeout(); assertTrue(aboutToAcquireWrite.get()); assertTrue(read1Acquired.get()); assertTrue(read1Released.get()); @@ -296,10 +294,11 @@ public class ApplicationImplTest extends LightPlatformTestCase { holdWrite.set(false); - while (!read2Released.get() && ok()); + while (!read2Released.get()) checkTimeout(); timeout = System.currentTimeMillis() + 2_000; - while (System.currentTimeMillis() < timeout && ok()) { + while (System.currentTimeMillis() < timeout) { + checkTimeout(); assertTrue(aboutToAcquireWrite.get()); assertTrue(read1Acquired.get()); assertTrue(read1Released.get()); @@ -326,7 +325,8 @@ public class ApplicationImplTest extends LightPlatformTestCase { LOG.debug("write lock acquired"); writeAcquired.set(true); - while (holdWrite.get() && ok()) { + while (holdWrite.get()) { + checkTimeout(); assertTrue(application.isWriteActionInProgress()); assertTrue(application.isWriteAccessAllowed()); assertFalse(application.isWriteActionPending()); @@ -362,7 +362,7 @@ public class ApplicationImplTest extends LightPlatformTestCase { ApplicationManager.getApplication().runReadAction((ThrowableComputable)() -> { LOG.append("inside read action\n"); readStarted = true; - while (!tryingToStartWriteAction && ok()) ; + while (!tryingToStartWriteAction) checkTimeout(); TimeoutUtil.sleep(100); readThreads = ContainerUtil.map(anotherReadActionStarted, readActionStarted -> new Thread(() -> { @@ -387,7 +387,7 @@ public class ApplicationImplTest extends LightPlatformTestCase { readThreads.forEach(Thread::start); for (AtomicBoolean threadStarted : anotherThreadStarted) { - while (!threadStarted.get() && ok()) ; + while (!threadStarted.get()) checkTimeout(); } // now the other threads try to get read lock. we should not let them for (int i = 0; i < 10; i++) { @@ -408,7 +408,7 @@ public class ApplicationImplTest extends LightPlatformTestCase { main.start(); - while (!readStarted && ok()); + while (!readStarted) checkTimeout(); tryingToStartWriteAction = true; LOG.append("\nwrite about to start"); ApplicationManager.getApplication().runWriteAction(() -> { @@ -440,7 +440,6 @@ public class ApplicationImplTest extends LightPlatformTestCase { assertFalse(ApplicationManager.getApplication().isReadAccessAllowed()); assertFalse(ApplicationManager.getApplication().isDispatchThread()); for (int i=0; i<100;i++) { - //noinspection SSBasedInspection SwingUtilities.invokeLater(() -> ApplicationManager.getApplication().runWriteAction(() -> TimeoutUtil.sleep(20))); ApplicationManager.getApplication().runReadAction(() -> TimeoutUtil.sleep(20)); } @@ -505,7 +504,6 @@ public class ApplicationImplTest extends LightPlatformTestCase { } public void testRunProcessWithProgressSynchronouslyInReadActionWithPendingWriteAction() throws Throwable { - //noinspection SSBasedInspection SwingUtilities.invokeLater(() -> ApplicationManager.getApplication().runWriteAction(EmptyRunnable.getInstance())); AtomicBoolean ran = new AtomicBoolean(); boolean result = ((ApplicationEx)ApplicationManager.getApplication()) @@ -714,7 +712,7 @@ public class ApplicationImplTest extends LightPlatformTestCase { app.runReadAction(() -> { readAcquired.set(true); try { - while (!stopRead.get() && ok()); + while (!stopRead.get()) checkTimeout(); } catch (Throwable e) { exception = e; @@ -724,14 +722,20 @@ public class ApplicationImplTest extends LightPlatformTestCase { } }) ); - while (!readAcquired.get()); + while (!readAcquired.get()) checkTimeout(); + + AtomicBoolean writeCompleted = new AtomicBoolean(); Future readAction2 = app.executeOnPooledThread(() -> { try { // wait for write action attempt to start - i.e. app.myLock.writeLock() started to execute - while (!app.myLock.writeRequested && ok()); + while (!app.myLock.writeRequested) checkTimeout(); app.executeByImpatientReader(() -> { try { + assertFalse(app.isReadAccessAllowed()); app.runReadAction(EmptyRunnable.getInstance()); + assertFalse(writeCompleted.get()); + if (exception != null) throw new RuntimeException(exception); + System.out.println(ThreadDumper.dumpThreadsToString()); fail("Must have been failed"); } catch (ApplicationUtil.CannotRunReadActionException ignored) { @@ -751,6 +755,7 @@ public class ApplicationImplTest extends LightPlatformTestCase { }); app.runWriteAction(EmptyRunnable.getInstance()); + writeCompleted.set(true); readAction2.get(); readAction1.get(); @@ -765,7 +770,7 @@ public class ApplicationImplTest extends LightPlatformTestCase { app.runReadAction(() -> { readAcquired.set(true); try { - while (!stopRead.get() && ok()) ; + while (!stopRead.get()) checkTimeout(); } catch (Throwable e) { exception = e; @@ -775,14 +780,20 @@ public class ApplicationImplTest extends LightPlatformTestCase { } }) ); - while (!readAcquired.get()); + while (!readAcquired.get()) checkTimeout(); AtomicBoolean executingImpatientReader = new AtomicBoolean(); - AtomicBoolean readAction2CalledReadAction = new AtomicBoolean(); Future readAction2 = app.executeOnPooledThread(() -> { // wait for write action attempt to start - while (!app.myLock.writeRequested); + while (!app.myLock.writeRequested) { + try { + checkTimeout(); + } + catch (Throwable e) { + throw new RuntimeException(e); + } + } ProgressManager.getInstance().executeNonCancelableSection(()-> app.executeByImpatientReader(() -> { executingImpatientReader.set(true); app.runReadAction(EmptyRunnable.getInstance()); @@ -792,7 +803,7 @@ public class ApplicationImplTest extends LightPlatformTestCase { Future readAction1Canceler = app.executeOnPooledThread(() -> { try { - while (!executingImpatientReader.get() && ok()); + while (!executingImpatientReader.get()) checkTimeout(); // make sure readAction2 does call runReadAction() TimeoutUtil.sleep(300); stopRead.set(true);