From cfb7d9af765c961103ce847fc50befb253231cc3 Mon Sep 17 00:00:00 2001 From: Julia Beliaeva Date: Mon, 5 Sep 2016 18:32:24 +0300 Subject: [PATCH] [vcs-log] cancel all progresses on dispose and throw PCE from disposed log storage It is bad to work with VcsLogStorage after dispose. This can happen if some refreshing/filtering processes are still in progress when log is disposed. So: 1. All processes are cancelled in VcsLogProgress. 2. VcsLogStorage throws PCE if accessed when disposed. --- .../com/intellij/vcs/log/data/VcsLogData.java | 2 +- .../intellij/vcs/log/data/VcsLogProgress.java | 32 ++++++++++++------- .../vcs/log/data/VcsLogRefresherImpl.java | 6 +++- .../vcs/log/data/VcsLogStorageImpl.java | 13 ++++++++ .../vcs/log/data/VcsLogRefresherTest.java | 3 +- 5 files changed, 42 insertions(+), 14 deletions(-) diff --git a/platform/vcs-log/impl/src/com/intellij/vcs/log/data/VcsLogData.java b/platform/vcs-log/impl/src/com/intellij/vcs/log/data/VcsLogData.java index 32a5024fe858..3f39e50df904 100644 --- a/platform/vcs-log/impl/src/com/intellij/vcs/log/data/VcsLogData.java +++ b/platform/vcs-log/impl/src/com/intellij/vcs/log/data/VcsLogData.java @@ -93,7 +93,7 @@ public class VcsLogData implements Disposable, VcsLogDataProvider { if (!(e instanceof ProcessCanceledException)) { LOG.error(e); } - }, RECENT_COMMITS_COUNT); + }, RECENT_COMMITS_COUNT, this); myContainingBranchesGetter = new ContainingBranchesGetter(this, this); } diff --git a/platform/vcs-log/impl/src/com/intellij/vcs/log/data/VcsLogProgress.java b/platform/vcs-log/impl/src/com/intellij/vcs/log/data/VcsLogProgress.java index b208e68f6088..fb8f9f0a704b 100644 --- a/platform/vcs-log/impl/src/com/intellij/vcs/log/data/VcsLogProgress.java +++ b/platform/vcs-log/impl/src/com/intellij/vcs/log/data/VcsLogProgress.java @@ -26,12 +26,13 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import java.util.List; +import java.util.Set; import java.util.function.Consumer; -public class VcsLogProgress { +public class VcsLogProgress implements Disposable { @NotNull private final Object myLock = new Object(); @NotNull private final List myListeners = ContainerUtil.newArrayList(); - private int myRunningTasksCount = 0; + @NotNull private Set myRunningTasks = ContainerUtil.newHashSet(); @NotNull public ProgressIndicator createProgressIndicator() { @@ -59,21 +60,21 @@ public class VcsLogProgress { public boolean isRunning() { synchronized (myLock) { - return myRunningTasksCount > 0; + return !myRunningTasks.isEmpty(); } } - private void started() { + private void started(@NotNull ProgressIndicator indicator) { synchronized (myLock) { - myRunningTasksCount++; - if (myRunningTasksCount == 1) fireNotification(ProgressListener::progressStarted); + myRunningTasks.add(indicator); + if (myRunningTasks.size() == 1) fireNotification(ProgressListener::progressStarted); } } - private void stopped() { + private void stopped(@NotNull ProgressIndicator indicator) { synchronized (myLock) { - myRunningTasksCount--; - if (myRunningTasksCount == 0) fireNotification(ProgressListener::progressStopped); + myRunningTasks.remove(indicator); + if (myRunningTasks.isEmpty()) fireNotification(ProgressListener::progressStopped); } } @@ -84,17 +85,26 @@ public class VcsLogProgress { } } + @Override + public void dispose() { + synchronized (myLock) { + for (ProgressIndicator indicator : myRunningTasks) { + indicator.cancel(); + } + } + } + private class VcsLogProgressIndicator extends AbstractProgressIndicatorBase { @Override public synchronized void start() { super.start(); - started(); + started(this); } @Override public synchronized void stop() { super.stop(); - stopped(); + stopped(this); } } diff --git a/platform/vcs-log/impl/src/com/intellij/vcs/log/data/VcsLogRefresherImpl.java b/platform/vcs-log/impl/src/com/intellij/vcs/log/data/VcsLogRefresherImpl.java index 01666e67682e..8dbbf2562d4c 100644 --- a/platform/vcs-log/impl/src/com/intellij/vcs/log/data/VcsLogRefresherImpl.java +++ b/platform/vcs-log/impl/src/com/intellij/vcs/log/data/VcsLogRefresherImpl.java @@ -15,11 +15,13 @@ */ package com.intellij.vcs.log.data; +import com.intellij.openapi.Disposable; import com.intellij.openapi.diagnostic.Logger; import com.intellij.openapi.progress.ProgressIndicator; import com.intellij.openapi.progress.ProgressManager; import com.intellij.openapi.progress.Task; import com.intellij.openapi.project.Project; +import com.intellij.openapi.util.Disposer; import com.intellij.openapi.util.Pair; import com.intellij.openapi.vcs.VcsException; import com.intellij.openapi.vfs.VirtualFile; @@ -65,7 +67,8 @@ public class VcsLogRefresherImpl implements VcsLogRefresher { @NotNull TopCommitsCache topCommitsDetailsCache, @NotNull Consumer dataPackUpdateHandler, @NotNull Consumer exceptionHandler, - int recentCommitsCount) { + int recentCommitsCount, + @NotNull Disposable parentDisposable) { myProject = project; myHashMap = hashMap; myProviders = providers; @@ -74,6 +77,7 @@ public class VcsLogRefresherImpl implements VcsLogRefresher { myExceptionHandler = exceptionHandler; myRecentCommitCount = recentCommitsCount; myProgress = new VcsLogProgress(); + Disposer.register(parentDisposable, myProgress); mySingleTaskController = new SingleTaskController(dataPack -> { myDataPack = dataPack; diff --git a/platform/vcs-log/impl/src/com/intellij/vcs/log/data/VcsLogStorageImpl.java b/platform/vcs-log/impl/src/com/intellij/vcs/log/data/VcsLogStorageImpl.java index 507a646184a3..a1921a9fda7b 100644 --- a/platform/vcs-log/impl/src/com/intellij/vcs/log/data/VcsLogStorageImpl.java +++ b/platform/vcs-log/impl/src/com/intellij/vcs/log/data/VcsLogStorageImpl.java @@ -17,6 +17,7 @@ package com.intellij.vcs.log.data; import com.intellij.openapi.Disposable; import com.intellij.openapi.diagnostic.Logger; +import com.intellij.openapi.progress.ProcessCanceledException; import com.intellij.openapi.project.Project; import com.intellij.openapi.util.Condition; import com.intellij.openapi.util.Disposer; @@ -60,6 +61,7 @@ public class VcsLogStorageImpl implements Disposable, VcsLogStorage { @NotNull private final PersistentEnumeratorBase myCommitIdEnumerator; @NotNull private final PersistentEnumeratorBase myRefsEnumerator; @NotNull private final Consumer myExceptionReporter; + private volatile boolean myDisposed = false; public VcsLogStorageImpl(@NotNull Project project, @NotNull Map logProviders, @@ -94,6 +96,7 @@ public class VcsLogStorageImpl implements Disposable, VcsLogStorage { @Override public int getCommitIndex(@NotNull Hash hash, @NotNull VirtualFile root) { + checkDisposed(); try { return getOrPut(hash, root); } @@ -106,6 +109,7 @@ public class VcsLogStorageImpl implements Disposable, VcsLogStorage { @Override @Nullable public CommitId getCommitId(int commitIndex) { + checkDisposed(); try { CommitId commitId = doGetCommitId(commitIndex); if (commitId == null) { @@ -122,6 +126,7 @@ public class VcsLogStorageImpl implements Disposable, VcsLogStorage { @Override @Nullable public CommitId findCommitId(@NotNull final Condition condition) { + checkDisposed(); try { final Ref hashRef = Ref.create(); myCommitIdEnumerator.iterateData(new CommonProcessors.FindProcessor() { @@ -144,6 +149,7 @@ public class VcsLogStorageImpl implements Disposable, VcsLogStorage { @Override public int getRefIndex(@NotNull VcsRef ref) { + checkDisposed(); try { return myRefsEnumerator.enumerate(ref); } @@ -156,6 +162,7 @@ public class VcsLogStorageImpl implements Disposable, VcsLogStorage { @Nullable @Override public VcsRef getVcsRef(int refIndex) { + checkDisposed(); try { return myRefsEnumerator.valueOf(refIndex); } @@ -166,6 +173,7 @@ public class VcsLogStorageImpl implements Disposable, VcsLogStorage { } public void flush() { + checkDisposed(); myCommitIdEnumerator.force(); myRefsEnumerator.force(); } @@ -173,6 +181,7 @@ public class VcsLogStorageImpl implements Disposable, VcsLogStorage { @Override public void dispose() { try { + myDisposed = true; myCommitIdEnumerator.close(); myRefsEnumerator.close(); } @@ -181,6 +190,10 @@ public class VcsLogStorageImpl implements Disposable, VcsLogStorage { } } + private void checkDisposed() { + if (myDisposed) throw new ProcessCanceledException(); + } + private static class MyCommitIdKeyDescriptor implements KeyDescriptor { @NotNull private final List myRoots; @NotNull private final TObjectIntHashMap myRootsReversed; diff --git a/platform/vcs-log/impl/test/com/intellij/vcs/log/data/VcsLogRefresherTest.java b/platform/vcs-log/impl/test/com/intellij/vcs/log/data/VcsLogRefresherTest.java index 65da9a2516e3..780e1f55b759 100644 --- a/platform/vcs-log/impl/test/com/intellij/vcs/log/data/VcsLogRefresherTest.java +++ b/platform/vcs-log/impl/test/com/intellij/vcs/log/data/VcsLogRefresherTest.java @@ -202,7 +202,8 @@ public class VcsLogRefresherTest extends VcsPlatformTest { myLogData = new VcsLogData(myProject, myLogProviders, LOG::error); Disposer.register(myProject, myLogData); return new VcsLogRefresherImpl(myProject, myLogData.getHashMap(), myLogProviders, myLogData.getUserRegistry(), - myLogData.getTopCommitsCache(), dataPackConsumer, FAILING_EXCEPTION_HANDLER, RECENT_COMMITS_COUNT) { + myLogData.getTopCommitsCache(), dataPackConsumer, FAILING_EXCEPTION_HANDLER, RECENT_COMMITS_COUNT, + myLogData) { @Override protected void startNewBackgroundTask(@NotNull final Task.Backgroundable refreshTask) { LOG.debug("Starting a background task...");