From 2e2a4d5f6e90678df1404b52624bd3858251e13f Mon Sep 17 00:00:00 2001 From: peter Date: Thu, 7 Apr 2016 19:24:21 +0200 Subject: [PATCH] allow write actions from app invokeLater used in an orthodox way, this way most code won't need rewriting to transactions --- .../application/TransactionGuardImpl.java | 40 ++++++++++-- .../application/impl/ApplicationImpl.java | 10 +-- .../application/impl/LaterInvocator.java | 2 +- .../application/TransactionTest.groovy | 62 ++++++++++++++----- 4 files changed, 89 insertions(+), 25 deletions(-) diff --git a/platform/core-impl/src/com/intellij/openapi/application/TransactionGuardImpl.java b/platform/core-impl/src/com/intellij/openapi/application/TransactionGuardImpl.java index b0980f4d3627..bc2381f901bc 100644 --- a/platform/core-impl/src/com/intellij/openapi/application/TransactionGuardImpl.java +++ b/platform/core-impl/src/com/intellij/openapi/application/TransactionGuardImpl.java @@ -27,8 +27,10 @@ import com.intellij.util.containers.ContainerUtil; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import java.util.Collections; import java.util.Map; import java.util.Queue; +import java.util.Set; import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.atomic.AtomicLong; @@ -38,10 +40,15 @@ import java.util.concurrent.atomic.AtomicLong; public class TransactionGuardImpl extends TransactionGuard { private static final Logger LOG = Logger.getInstance("#com.intellij.openapi.application.TransactionGuardImpl"); private final Queue myQueue = new LinkedBlockingQueue(); - private final Map myModalities = ContainerUtil.createConcurrentWeakMap(); + private final Map myModality2Transaction = ContainerUtil.createConcurrentWeakMap(); + private final Set myWriteSafeModalities = Collections.newSetFromMap(ContainerUtil.createConcurrentWeakMap()); private TransactionIdImpl myCurrentTransaction; private boolean myWritingAllowed; + public TransactionGuardImpl() { + myWriteSafeModalities.add(ModalityState.NON_MODAL); + } + @Override @NotNull public AccessToken startSynchronousTransaction(@NotNull TransactionKind kind) throws IllegalStateException { @@ -263,7 +270,7 @@ public class TransactionGuardImpl extends TransactionGuard { } public boolean isWriteActionAllowed() { - return !Registry.is("ide.require.transaction.for.model.changes", false) || isInsideTransaction() || myWritingAllowed; + return !Registry.is("ide.require.transaction.for.model.changes", false) || myWritingAllowed; } @Override @@ -282,7 +289,7 @@ public class TransactionGuardImpl extends TransactionGuard { public TransactionIdImpl getContextTransaction() { if (!ApplicationManager.getApplication().isDispatchThread()) { ProgressIndicator indicator = ProgressIndicatorProvider.getGlobalProgressIndicator(); - return indicator != null ? myModalities.get(indicator.getModalityState()) : null; + return indicator != null ? myModality2Transaction.get(indicator.getModalityState()) : null; } return myWritingAllowed ? myCurrentTransaction : null; @@ -291,13 +298,36 @@ public class TransactionGuardImpl extends TransactionGuard { public void enteredModality(@NotNull ModalityState modality) { TransactionIdImpl contextTransaction = getContextTransaction(); if (contextTransaction != null) { - myModalities.put(modality, contextTransaction); + myModality2Transaction.put(modality, contextTransaction); + } + if (myWritingAllowed) { + myWriteSafeModalities.add(modality); } } @Nullable public TransactionIdImpl getModalityTransaction(@NotNull ModalityState modalityState) { - return myModalities.get(modalityState); + return myModality2Transaction.get(modalityState); + } + + @NotNull + public Runnable wrapLaterInvocation(@NotNull final Runnable runnable, @NotNull ModalityState modalityState) { + if (myWriteSafeModalities.contains(modalityState)) { + return new Runnable() { + @Override + public void run() { + final boolean prev = myWritingAllowed; + myWritingAllowed = true; + try { + runnable.run(); + } finally { + myWritingAllowed = prev; + } + } + }; + } + + return runnable; } private static class Transaction { 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 18e297a0262c..2e6ddbdd01b2 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 @@ -435,22 +435,22 @@ public class ApplicationImpl extends PlatformComponentManagerImpl implements App @Override public void invokeLater(@NotNull final Runnable runnable) { - myInvokator.invokeLater(runnable); + invokeLater(runnable, getDisposed()); } @Override public void invokeLater(@NotNull final Runnable runnable, @NotNull final Condition expired) { - myInvokator.invokeLater(runnable, expired); + invokeLater(runnable, ModalityState.defaultModalityState(), expired); } @Override public void invokeLater(@NotNull final Runnable runnable, @NotNull final ModalityState state) { - myInvokator.invokeLater(runnable, state); + invokeLater(runnable, state, getDisposed()); } @Override public void invokeLater(@NotNull final Runnable runnable, @NotNull final ModalityState state, @NotNull final Condition expired) { - myInvokator.invokeLater(runnable, state, expired); + myInvokator.invokeLater(((TransactionGuardImpl)TransactionGuard.getInstance()).wrapLaterInvocation(runnable, state), state, expired); } @Override @@ -737,7 +737,7 @@ public class ApplicationImpl extends PlatformComponentManagerImpl implements App LOG.error("Calling invokeAndWait from read-action leads to possible deadlock."); } - LaterInvocator.invokeAndWait(runnable, modalityState); + LaterInvocator.invokeAndWait(((TransactionGuardImpl)TransactionGuard.getInstance()).wrapLaterInvocation(runnable, modalityState), modalityState); } @Override diff --git a/platform/platform-impl/src/com/intellij/openapi/application/impl/LaterInvocator.java b/platform/platform-impl/src/com/intellij/openapi/application/impl/LaterInvocator.java index 3ccb7f7b1caa..981b9f101712 100644 --- a/platform/platform-impl/src/com/intellij/openapi/application/impl/LaterInvocator.java +++ b/platform/platform-impl/src/com/intellij/openapi/application/impl/LaterInvocator.java @@ -236,7 +236,7 @@ public class LaterInvocator { } @TestOnly - static void leaveAllModals() { + public static void leaveAllModals() { ourModalEntities.clear(); while (ourModalityStack.size() > 1) { ourModalityStack.pop(); diff --git a/platform/platform-tests/testSrc/com/intellij/application/TransactionTest.groovy b/platform/platform-tests/testSrc/com/intellij/application/TransactionTest.groovy index 70a4d91abbd7..128abf2aea82 100644 --- a/platform/platform-tests/testSrc/com/intellij/application/TransactionTest.groovy +++ b/platform/platform-tests/testSrc/com/intellij/application/TransactionTest.groovy @@ -1,10 +1,7 @@ package com.intellij.application -import com.intellij.openapi.application.Application -import com.intellij.openapi.application.ApplicationManager -import com.intellij.openapi.application.TransactionGuard -import com.intellij.openapi.application.TransactionGuardImpl -import com.intellij.openapi.application.TransactionId +import com.intellij.openapi.application.* +import com.intellij.openapi.application.impl.LaterInvocator import com.intellij.openapi.progress.ProgressManager import com.intellij.openapi.util.Disposer import com.intellij.openapi.util.registry.Registry @@ -38,6 +35,7 @@ class TransactionTest extends LightPlatformTestCase { protected void tearDown() throws Exception { Registry.get("ide.require.transaction.for.model.changes").resetToDefault() log.clear() + LaterInvocator.leaveAllModals() super.tearDown() } @@ -45,18 +43,23 @@ class TransactionTest extends LightPlatformTestCase { assert app.isDispatchThread() assert !app.isWriteAccessAllowed() - SwingUtilities.invokeLater { - try { - app.runWriteAction {} - fail() - } - catch (AssertionError ignore) { - // a trace is also printed to stderr, which is expected - } - } + SwingUtilities.invokeLater { assertWritingProhibited() } UIUtil.dispatchAllInvocationEvents() } + private void assertWritingProhibited() { + boolean writeActionFailed = false + try { + app.runWriteAction { log << 'writing' } + } + catch (AssertionError ignore) { + writeActionFailed = true + } + if (!writeActionFailed) { + fail('write action should fail') + } + } + public void "test parent disposable"() { def parent = Disposer.newDisposable() @@ -176,4 +179,35 @@ class TransactionTest extends LightPlatformTestCase { } } + public void "test write access in modal invokeLater"() { + LaterInvocator.enterModal(new Object()) + UIUtil.dispatchAllInvocationEvents() + def unsafeModality = ModalityState.current() + + TransactionGuard.submitTransaction testRootDisposable, { + log << '1' + + LaterInvocator.enterModal(new Object()) + def safeModality = ModalityState.current() + app.executeOnPooledThread({ + app.invokeLater({ + app.runWriteAction { log << '2' } + }, safeModality) + app.invokeLater({ + assertWritingProhibited() + log << '4' + }, unsafeModality) + app.invokeLater({ + assertWritingProhibited() + log << '3' + }, ModalityState.any()) + app.invokeLater({ app.runWriteAction { log << '5' } }, ModalityState.NON_MODAL) + }).get() + UIUtil.dispatchAllInvocationEvents() + LaterInvocator.leaveAllModals() + UIUtil.dispatchAllInvocationEvents() + assert log == ['1', '2', '3', '4', '5'] + } + } + }