From 34525b4c72536ccc291a14ab9c47853f57cee5e3 Mon Sep 17 00:00:00 2001 From: peter Date: Tue, 29 Mar 2016 11:54:20 +0200 Subject: [PATCH] allow transactions to be accompanied by parentDisposable --- .../options/CompileStepBeforeRun.java | 2 +- .../openapi/application/TransactionGuard.java | 46 +++++++++---- .../openapi/application/TransactionKind.java | 3 +- .../application/TransactionGuardImpl.java | 15 +++- .../psi/impl/DocumentCommitThread.java | 3 +- .../impl/PushedFilePropertiesUpdaterImpl.java | 10 +-- .../FileBasedIndexProjectHandler.java | 5 +- .../application/TransactionTest.groovy | 68 +++++++++++++++++++ .../testFramework/LoggedErrorProcessor.java | 23 +++++-- .../com/intellij/openapi/util/Disposer.java | 2 +- .../openapi/util/objectTree/ObjectTree.java | 24 +++++-- .../openapi/vcs/ex/LineStatusTracker.java | 2 +- 12 files changed, 162 insertions(+), 41 deletions(-) create mode 100644 platform/platform-tests/testSrc/com/intellij/application/TransactionTest.groovy diff --git a/java/execution/impl/src/com/intellij/compiler/options/CompileStepBeforeRun.java b/java/execution/impl/src/com/intellij/compiler/options/CompileStepBeforeRun.java index 5cbaf8aaabec..1723cb388b40 100644 --- a/java/execution/impl/src/com/intellij/compiler/options/CompileStepBeforeRun.java +++ b/java/execution/impl/src/com/intellij/compiler/options/CompileStepBeforeRun.java @@ -141,7 +141,7 @@ public class CompileStepBeforeRun extends BeforeRunTaskProvider @@ -41,7 +43,7 @@ import org.jetbrains.annotations.NotNull; * outer transaction has shown a dialog with an editor, and typing into that editor (which requires a transaction for changing document) * should be allowed. For such cases, the framework should be notified which transaction kinds are allowed to merged into * the main transaction and executed immediately. Use {@link #acceptNestedTransactions(TransactionKind...)} for that. Inner transactions - * should be given some kind in such circumstances: {@link #submitMergeableTransaction(TransactionKind, Runnable)}. + * should be given some kind in such circumstances: {@link #submitMergeableTransaction(Disposable, TransactionKind, Runnable)}. * *

FAQ

* @@ -69,8 +71,8 @@ import org.jetbrains.annotations.NotNull; * project: they'd be blocked by the running transaction. * * Q: I've got "Write access is allowed from model transactions only" exception, what do I do?
- * A: You're likely inside an "invokeLater"-like call. Please consider replacing it with {@link #submitTransaction(Runnable)} or - * {@link #submitMergeableTransaction(TransactionKind, Runnable)}. + * A: You're likely inside an "invokeLater"-like call. Please consider replacing it with {@link #submitTransaction(Disposable, Runnable)} or + * {@link #submitMergeableTransaction(Disposable, TransactionKind, Runnable)} *

* * Q: I've got "Nested transactions are not allowed" exception, what do I do?
@@ -82,9 +84,9 @@ import org.jetbrains.annotations.NotNull; * (for text field editing inside the dialogs) but not to * other model changes, e.g. root changes. The outer transaction code might then specify which kinds it's prepared to (by using * {@link #acceptNestedTransactions(TransactionKind...)}), and the inner transaction code should have the very same transaction kind - * (by using {@link #submitMergeableTransaction(TransactionKind, Runnable)} or {@link #startSynchronousTransaction(TransactionKind)}). - * If the nested transaction is not expected by the outer code, it must be made asynchronous by using either {@link #submitTransaction(Runnable)} - * or {@link #submitMergeableTransaction(TransactionKind, Runnable)}. + * (by using {@link #submitMergeableTransaction(Disposable, TransactionKind, Runnable)} or {@link #startSynchronousTransaction(TransactionKind)}). + * If the nested transaction is not expected by the outer code, it must be made asynchronous by using either {@link #submitTransaction(Disposable, Runnable)} + * or {@link #submitMergeableTransaction(Disposable, TransactionKind, Runnable)}. *

* * Q: What's the difference between transactions and read/write actions and commands ({@link com.intellij.openapi.command.CommandProcessor})?
@@ -103,17 +105,27 @@ public abstract class TransactionGuard { return ServiceManager.getService(TransactionGuard.class); } + /** + * Same as {@link #submitTransaction(Disposable, Runnable)}, but without any parent disposable. + */ + public static void submitTransaction(@NotNull Runnable transaction) { + submitTransaction(null, transaction); + } + /** * Ensures that some code will be run in a transaction. It's guaranteed that no other transactions are run at the same time. * The code will be run on Swing thread immediately or after all other queued transactions (if any) have been completed.

* - * For more advanced version, see {@link #submitMergeableTransaction(TransactionKind, Runnable)}. + * For more advanced version, see {@link #submitMergeableTransaction(Disposable, TransactionKind, Runnable)} * Transactions submitted via this method use {@link TransactionKind#ANY_CHANGE} kind. * + * @param parentDisposable an object whose disposing (via {@link com.intellij.openapi.util.Disposer} makes this transaction invalid, + * and so it won't be run after it has been disposed. Can be null, that would mean the transaction can be run + * until the application is disposed. * @param transaction code to execute inside a transaction. */ - public static void submitTransaction(@NotNull Runnable transaction) { - getInstance().submitMergeableTransaction(TransactionKind.ANY_CHANGE, transaction); + public static void submitTransaction(@Nullable Disposable parentDisposable, @NotNull Runnable transaction) { + getInstance().submitMergeableTransaction(parentDisposable, TransactionKind.ANY_CHANGE, transaction); } /** @@ -132,7 +144,7 @@ public abstract class TransactionGuard { /** * Schedules a transaction and waits for it to be completed. Fails if invoked on UI thread inside an incompatible transaction, * or inside a read action on non-UI thread. - * @see #submitMergeableTransaction(TransactionKind, Runnable) + * @see #submitMergeableTransaction(Disposable, TransactionKind, Runnable) * @throws ProcessCanceledException if current thread is interrupted */ public abstract void submitTransactionAndWait(@NotNull TransactionKind kind, @NotNull Runnable transaction) throws ProcessCanceledException; @@ -152,22 +164,32 @@ public abstract class TransactionGuard { } /** - * A synchronous version of {@link #submitMergeableTransaction(TransactionKind, Runnable)}. + * A synchronous version of {@link #submitMergeableTransaction(Disposable, TransactionKind, Runnable)}. * @return a token object for this transaction. Call {@link AccessToken#finish()} (inside finally) when the transaction is complete. */ @NotNull public abstract AccessToken startSynchronousTransaction(@NotNull TransactionKind kind); + /** + * Same as {@link #submitMergeableTransaction(Disposable, TransactionKind, Runnable)} with no parent disposable. + */ + public void submitMergeableTransaction(@NotNull TransactionKind kind, @NotNull Runnable transaction) { + submitMergeableTransaction(null, kind, transaction); + } + /** * When on UI thread and there's no other transaction running, executes the given runnable. If there is a transaction running, * but the given {@code kind} is allowed via {@link #acceptNestedTransactions(TransactionKind...)}, merges two transactions * and executes the provided code immediately. Otherwise * adds the runnable to a queue. When all transactions scheduled before this one are finished, executes the given * runnable under a transaction. + * @param parentDisposable an object whose disposing (via {@link com.intellij.openapi.util.Disposer} makes this transaction invalid, + * and so it won't be run after it has been disposed. Can be null, that would mean the transaction can be run + * until the application is disposed. * @param kind a "kind" object for transaction merging * @param transaction code to execute inside a transaction. */ - public abstract void submitMergeableTransaction(@NotNull TransactionKind kind, @NotNull Runnable transaction); + public abstract void submitMergeableTransaction(@Nullable Disposable parentDisposable, @NotNull TransactionKind kind, @NotNull Runnable transaction); /** * Allow incoming transactions of the specified kinds to be executed immediately, instead of being queued until the current transaction is finished.

diff --git a/platform/core-api/src/com/intellij/openapi/application/TransactionKind.java b/platform/core-api/src/com/intellij/openapi/application/TransactionKind.java index 18660fe5622d..dccdaf8056c8 100644 --- a/platform/core-api/src/com/intellij/openapi/application/TransactionKind.java +++ b/platform/core-api/src/com/intellij/openapi/application/TransactionKind.java @@ -15,10 +15,11 @@ */ package com.intellij.openapi.application; +import com.intellij.openapi.Disposable; import com.intellij.openapi.editor.Document; /** - * A kind of transaction used in {@link TransactionGuard#submitMergeableTransaction(TransactionKind, Runnable)}, + * A kind of transaction used in {@link TransactionGuard#submitMergeableTransaction(Disposable, TransactionKind, Runnable)}, * {@link TransactionGuard#acceptNestedTransactions(TransactionKind...)}. */ public interface TransactionKind { 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 3772a202b304..8de10fd485c2 100644 --- a/platform/core-impl/src/com/intellij/openapi/application/TransactionGuardImpl.java +++ b/platform/core-impl/src/com/intellij/openapi/application/TransactionGuardImpl.java @@ -15,14 +15,17 @@ */ package com.intellij.openapi.application; +import com.intellij.openapi.Disposable; import com.intellij.openapi.diagnostic.Attachment; import com.intellij.openapi.diagnostic.Logger; import com.intellij.openapi.progress.ProcessCanceledException; +import com.intellij.openapi.util.Disposer; import com.intellij.openapi.util.registry.Registry; import com.intellij.psi.impl.DebugUtil; import com.intellij.util.concurrency.Semaphore; import com.intellij.util.containers.ContainerUtil; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; import java.util.List; import java.util.Queue; @@ -111,7 +114,15 @@ public class TransactionGuardImpl extends TransactionGuard { } @Override - public void submitMergeableTransaction(@NotNull final TransactionKind kind, @NotNull final Runnable transaction) { + public void submitMergeableTransaction(@Nullable final Disposable parentDisposable, @NotNull final TransactionKind kind, @NotNull final Runnable _transaction) { + @NotNull final Runnable transaction = parentDisposable == null ? _transaction : new Runnable() { + @Override + public void run() { + if (!Disposer.isDisposed(parentDisposable)) { + _transaction.run(); + } + } + }; Runnable runnable = new Runnable() { @Override public void run() { @@ -183,7 +194,7 @@ public class TransactionGuardImpl extends TransactionGuard { final Semaphore semaphore = new Semaphore(); semaphore.down(); final Throwable[] exception = {null}; - submitMergeableTransaction(kind, new Runnable() { + submitMergeableTransaction(null, kind, new Runnable() { @Override public void run() { try { diff --git a/platform/core-impl/src/com/intellij/psi/impl/DocumentCommitThread.java b/platform/core-impl/src/com/intellij/psi/impl/DocumentCommitThread.java index f593bc5ee271..d4e7adc18ed9 100644 --- a/platform/core-impl/src/com/intellij/psi/impl/DocumentCommitThread.java +++ b/platform/core-impl/src/com/intellij/psi/impl/DocumentCommitThread.java @@ -421,10 +421,11 @@ public class DocumentCommitThread implements Runnable, Disposable, DocumentCommi if (success) { assert !myApplication.isDispatchThread(); final Runnable finalFinishRunnable = finishRunnable; + final Project finalProject = project; myApplication.invokeLater(new Runnable() { @Override public void run() { - TransactionGuard.getInstance().submitMergeableTransaction(TransactionKind.TEXT_EDITING, finalFinishRunnable); + TransactionGuard.getInstance().submitMergeableTransaction(finalProject, TransactionKind.TEXT_EDITING, finalFinishRunnable); } }, task.myCreationModalityState); } diff --git a/platform/lang-impl/src/com/intellij/openapi/roots/impl/PushedFilePropertiesUpdaterImpl.java b/platform/lang-impl/src/com/intellij/openapi/roots/impl/PushedFilePropertiesUpdaterImpl.java index 47f392063e59..d9af7f6ef0bb 100644 --- a/platform/lang-impl/src/com/intellij/openapi/roots/impl/PushedFilePropertiesUpdaterImpl.java +++ b/platform/lang-impl/src/com/intellij/openapi/roots/impl/PushedFilePropertiesUpdaterImpl.java @@ -23,6 +23,7 @@ import com.intellij.ProjectTopics; import com.intellij.openapi.application.ApplicationManager; import com.intellij.openapi.application.ModalityState; import com.intellij.openapi.application.TransactionGuard; +import com.intellij.openapi.application.WriteAction; import com.intellij.openapi.diagnostic.Logger; import com.intellij.openapi.extensions.ExtensionException; import com.intellij.openapi.extensions.Extensions; @@ -408,16 +409,11 @@ public class PushedFilePropertiesUpdaterImpl extends PushedFilePropertiesUpdater private static void reloadPsi(final VirtualFile file, final Project project) { final FileManagerImpl fileManager = (FileManagerImpl)((PsiManagerEx)PsiManager.getInstance(project)).getFileManager(); if (fileManager.findCachedViewProvider(file) != null) { - Runnable runnable = () -> { - if (project.isDisposed()) { - return; - } - ApplicationManager.getApplication().runWriteAction(() -> fileManager.forceReload(file)); - }; + Runnable runnable = () -> WriteAction.run(() -> fileManager.forceReload(file)); if (ApplicationManager.getApplication().isDispatchThread()) { runnable.run(); } else { - TransactionGuard.submitTransaction(runnable); + TransactionGuard.submitTransaction(project, runnable); } } } 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 10ba085f3e92..df2bd37e7546 100644 --- a/platform/lang-impl/src/com/intellij/util/indexing/FileBasedIndexProjectHandler.java +++ b/platform/lang-impl/src/com/intellij/util/indexing/FileBasedIndexProjectHandler.java @@ -39,7 +39,6 @@ import com.intellij.openapi.vfs.VfsUtilCore; import com.intellij.openapi.vfs.VirtualFile; import com.intellij.openapi.vfs.VirtualFileVisitor; import com.intellij.util.Consumer; -import com.intellij.util.ui.UIUtil; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -82,8 +81,8 @@ public class FileBasedIndexProjectHandler extends AbstractProjectComponent imple PushedFilePropertiesUpdater.getInstance(project).initializeProperties(); // schedule dumb mode start after the read action we're currently in - TransactionGuard.submitTransaction(() -> { - if (!project.isDisposed() && FileBasedIndex.getInstance() instanceof FileBasedIndexImpl) { + TransactionGuard.submitTransaction(project, () -> { + if (FileBasedIndex.getInstance() instanceof FileBasedIndexImpl) { DumbService.getInstance(project).queueTask(new UnindexedFilesUpdater(project, true)); } }); diff --git a/platform/platform-tests/testSrc/com/intellij/application/TransactionTest.groovy b/platform/platform-tests/testSrc/com/intellij/application/TransactionTest.groovy new file mode 100644 index 000000000000..c744cc514fc4 --- /dev/null +++ b/platform/platform-tests/testSrc/com/intellij/application/TransactionTest.groovy @@ -0,0 +1,68 @@ +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.util.Disposer +import com.intellij.openapi.util.registry.Registry +import com.intellij.testFramework.LightPlatformTestCase +import com.intellij.testFramework.LoggedErrorProcessor +import com.intellij.util.ui.UIUtil + +import javax.swing.* +/** + * @author peter + */ +class TransactionTest extends LightPlatformTestCase { + static TransactionGuardImpl getGuard() { + return TransactionGuard.getInstance() as TransactionGuardImpl + } + + static Application getApp() { + return ApplicationManager.getApplication() + } + + @Override + protected void setUp() throws Exception { + super.setUp() + Registry.get("ide.require.transaction.for.model.changes").setValue(true) + LoggedErrorProcessor.instance.disableStderrDumping(testRootDisposable) + } + + @Override + protected void tearDown() throws Exception { + Registry.get("ide.require.transaction.for.model.changes").resetToDefault() + super.tearDown() + } + + public void "test write action in invokeLater requires transaction"() { + 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 + } + } + UIUtil.dispatchAllInvocationEvents() + } + + public void "test parent disposable"() { + def parent = Disposer.newDisposable() + def log = [] + + SwingUtilities.invokeLater { TransactionGuard.submitTransaction parent, { log << "1" } } + UIUtil.dispatchAllInvocationEvents() + assert log == ['1'] + + Disposer.dispose(parent) + SwingUtilities.invokeLater { TransactionGuard.submitTransaction parent, { log << "2" } } + UIUtil.dispatchAllInvocationEvents() + assert log == ['1'] + } +} diff --git a/platform/testFramework/src/com/intellij/testFramework/LoggedErrorProcessor.java b/platform/testFramework/src/com/intellij/testFramework/LoggedErrorProcessor.java index eb7611e4ed9e..032df5a0c728 100644 --- a/platform/testFramework/src/com/intellij/testFramework/LoggedErrorProcessor.java +++ b/platform/testFramework/src/com/intellij/testFramework/LoggedErrorProcessor.java @@ -15,6 +15,8 @@ */ package com.intellij.testFramework; +import com.intellij.openapi.Disposable; +import com.intellij.openapi.util.Disposer; import org.apache.log4j.Logger; import org.jetbrains.annotations.NotNull; @@ -22,6 +24,7 @@ public class LoggedErrorProcessor { private static final LoggedErrorProcessor DEFAULT = new LoggedErrorProcessor(); private static LoggedErrorProcessor ourInstance = DEFAULT; + private boolean myMirrorToStderr = true; @NotNull public static LoggedErrorProcessor getInstance() { @@ -44,15 +47,23 @@ public class LoggedErrorProcessor { public void processError(String message, Throwable t, String[] details, @NotNull Logger logger) { logger.info(message, t); - System.err.println("ERROR: " + message); - if (t != null) t.printStackTrace(System.err); - if (details != null && details.length > 0) { - System.out.println("details: "); - for (String detail : details) { - System.out.println(detail); + if (myMirrorToStderr) { + System.err.println("ERROR: " + message); + if (t != null) t.printStackTrace(System.err); + if (details != null && details.length > 0) { + System.out.println("details: "); + for (String detail : details) { + System.out.println(detail); + } } } throw new AssertionError(message); } + + public void disableStderrDumping(@NotNull Disposable parentDisposable) { + boolean prev = myMirrorToStderr; + myMirrorToStderr = false; + Disposer.register(parentDisposable, () -> myMirrorToStderr = prev); + } } diff --git a/platform/util/src/com/intellij/openapi/util/Disposer.java b/platform/util/src/com/intellij/openapi/util/Disposer.java index b136e5078d82..963e8c431877 100644 --- a/platform/util/src/com/intellij/openapi/util/Disposer.java +++ b/platform/util/src/com/intellij/openapi/util/Disposer.java @@ -104,7 +104,7 @@ public class Disposer { } public static boolean isDisposed(@NotNull Disposable disposable) { - return !ourTree.containsKey(disposable); + return ourTree.getDisposalInfo(disposable) != null; } public static Disposable get(@NotNull String key) { diff --git a/platform/util/src/com/intellij/openapi/util/objectTree/ObjectTree.java b/platform/util/src/com/intellij/openapi/util/objectTree/ObjectTree.java index d8612df82ef1..90d8b8adcf4e 100644 --- a/platform/util/src/com/intellij/openapi/util/objectTree/ObjectTree.java +++ b/platform/util/src/com/intellij/openapi/util/objectTree/ObjectTree.java @@ -60,13 +60,14 @@ public final class ObjectTree { } public final void register(@NotNull T parent, @NotNull T child) { + Object wasDisposed = getDisposalInfo(parent); + if (wasDisposed != null) { + throw new IncorrectOperationException("Sorry but parent: " + parent + " has already been disposed " + + "(see the cause for stacktrace) so the child: "+child+" will never be disposed", + wasDisposed instanceof Throwable ? (Throwable)wasDisposed : null); + } + synchronized (treeLock) { - Object wasDisposed = myDisposedObjects.get(parent); - if (wasDisposed != null) { - throw new IncorrectOperationException("Sorry but parent: " + parent + " has already been disposed " + - "(see the cause for stacktrace) so the child: "+child+" will never be disposed", - wasDisposed instanceof Throwable ? (Throwable)wasDisposed : null); - } myDisposedObjects.remove(child); // if we dispose thing and then register it back it means it's not disposed anymore ObjectNode parentNode = getNode(parent); if (parentNode == null) parentNode = createNodeFor(parent, null); @@ -91,6 +92,12 @@ public final class ObjectTree { } } + public Object getDisposalInfo(@NotNull T parent) { + synchronized (treeLock) { + return myDisposedObjects.get(parent); + } + } + private void checkWasNotAddedAlready(ObjectNode childNode, @NotNull ObjectNode parentNode) { for (ObjectNode node = childNode; node != null; node = node.getParent()) { if (node == parentNode) { @@ -120,6 +127,7 @@ public final class ObjectTree { } if (node == null) { if (processUnregistered) { + rememberDisposedTrace(object); executeUnregistered(object, action); return true; } @@ -245,6 +253,10 @@ public final class ObjectTree { for (ObjectTreeListener each : myListeners) { each.objectExecuted(object); } + rememberDisposedTrace(object); + } + + private void rememberDisposedTrace(@NotNull Object object) { synchronized (treeLock) { myDisposedObjects.put(object, Disposer.isDebugMode() ? ThrowableInterner.intern(new Throwable()) : Boolean.TRUE); } diff --git a/platform/vcs-impl/src/com/intellij/openapi/vcs/ex/LineStatusTracker.java b/platform/vcs-impl/src/com/intellij/openapi/vcs/ex/LineStatusTracker.java index 6ff2146adf49..24bd929b64c2 100644 --- a/platform/vcs-impl/src/com/intellij/openapi/vcs/ex/LineStatusTracker.java +++ b/platform/vcs-impl/src/com/intellij/openapi/vcs/ex/LineStatusTracker.java @@ -386,7 +386,7 @@ public class LineStatusTracker { private void markFileUnchanged() { // later to avoid saving inside document change event processing. - ApplicationManager.getApplication().invokeLater(() -> TransactionGuard.submitTransaction(() -> { + ApplicationManager.getApplication().invokeLater(() -> TransactionGuard.submitTransaction(myProject, () -> { FileDocumentManager.getInstance().saveDocument(myDocument); boolean stillEmpty; synchronized (myLock) {