diff --git a/ide/git/src/org/netbeans/modules/git/ui/diff/MultiDiffPanelController.java b/ide/git/src/org/netbeans/modules/git/ui/diff/MultiDiffPanelController.java index 2d6f978bb57a..9342e9a0cd56 100644 --- a/ide/git/src/org/netbeans/modules/git/ui/diff/MultiDiffPanelController.java +++ b/ide/git/src/org/netbeans/modules/git/ui/diff/MultiDiffPanelController.java @@ -47,6 +47,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.atomic.AtomicLong; import java.util.logging.Level; import java.util.logging.Logger; import java.util.prefs.PreferenceChangeEvent; @@ -167,6 +168,8 @@ public class MultiDiffPanelController implements ActionListener, PropertyChangeL private final ApplyChangesTask changeTask = new ApplyChangesTask(); private final RequestProcessor.Task refreshNodesRPTask = RP.create(refreshNodesTask); private final RequestProcessor.Task changeRPTask = RP.create(changeTask); + private final AtomicLong refreshGeneration = new AtomicLong(); + private volatile boolean closed; private static final String TEXT_DIFF = "text/x-diff"; //NOI18N private GitProgressSupport multiTextDiffSupport; private static final String PROP_SEARCH_CONTAINER = "diff.search.container"; //NOI18N @@ -266,7 +269,7 @@ public MultiDiffPanelController (File file, Revision rev1, Revision rev2, int re Map localSetups = Collections.singletonMap(file, s); setSetups(localSetups, getCookiesFromSetups(localSetups)); setDiffIndex(s, 0, false); - dpt = new DiffPrepareTask(setups.values().toArray(new Setup[setups.size()])); + dpt = new DiffPrepareTask(setups.values().toArray(new Setup[setups.size()]), refreshGeneration.get()); prepareTask = RP.create(dpt); prepareTask.schedule(0); } @@ -426,12 +429,18 @@ UndoRedo getUndoRedo () { } public void componentClosed () { + if (closed) { + return; + } + closed = true; + refreshGeneration.incrementAndGet(); + cancelBackgroundTasks(); setSetups(Collections.emptyMap(), Collections.emptyMap()); prevAction.setEnabled(false); nextAction.setEnabled(false); - cancelBackgroundTasks(); setups.clear(); editorCookies.clear(); + currentSetup = null; if (list != null) { Git.getInstance().getFileStatusCache().removePropertyChangeListener(list); @@ -448,6 +457,9 @@ private void cancelBackgroundTasks() { if (multiTextDiffSupport != null) { multiTextDiffSupport.cancel(); } + if (dpt != null) { + dpt.cancel(); + } if (prepareTask != null) { prepareTask.cancel(); } @@ -457,6 +469,11 @@ private void cancelBackgroundTasks() { } refreshNodesTask.cancel(); refreshNodesRPTask.cancel(); + changeTask.cancel(); + changeRPTask.cancel(); + if (refreshComboTask != null) { + refreshComboTask.cancel(); + } } void setFocused (boolean focused) { @@ -641,17 +658,24 @@ private void setDisplayStatuses (EnumSet displayStatuses) { } private void setSetups (Map setups, Map editorCookies) { - for (Map.Entry e : this.setups.entrySet()) { - Setup setup = e.getValue(); + closeSetups(this.setups); + this.setups.clear(); + this.setups.putAll(setups); + this.editorCookies.clear(); + this.editorCookies.putAll(editorCookies); + } + + private void closeSetups(Map setups) { + for (Setup setup : setups.values()) { if (setup != null) { + DiffController view = setup.getView(); + if (view != null) { + view.removePropertyChangeListener(this); + } setup.getFirstSource().close(); setup.getSecondSource().close(); } } - this.setups.clear(); - this.setups.putAll(setups); - this.editorCookies.clear(); - this.editorCookies.putAll(editorCookies); } private void onNextButton() { @@ -1274,7 +1298,8 @@ void filesSelected (File... files) { // private void refreshNodes () { - if (context != null) { + if (context != null && !closed) { + refreshGeneration.incrementAndGet(); synchronized (changes) { changes.clear(); } @@ -1295,7 +1320,7 @@ private void refreshNodes () { private void startPrepareTask (Setup activeSetup) { if (setups.size() > 0) { - dpt = new DiffPrepareTask(setups.values().toArray(new Setup[setups.size()])); + dpt = new DiffPrepareTask(setups.values().toArray(new Setup[setups.size()]), refreshGeneration.get()); dpt.setSelectedSetup(activeSetup); prepareTask = RP.create(dpt); prepareTask.schedule(0); @@ -1388,6 +1413,18 @@ private EditorCookie[] sort (Map cookies, DiffNode[] nodes) } private void setupsChanged (final Map newSetups, final List nodes) { + setupsChanged(newSetups, nodes, refreshGeneration.incrementAndGet()); + } + + private boolean isCurrent(long generation) { + return !closed && refreshGeneration.get() == generation; + } + + private void setupsChanged (final Map newSetups, final List nodes, final long generation) { + if (!isCurrent(generation)) { + closeSetups(newSetups); + return; + } final Map cookies = getCookiesFromSetups(newSetups); final DiffNode[] nodeArray = nodes.toArray(new DiffNode[0]); final Object modelDataList = fileListComponent.prepareModel(nodeArray); @@ -1395,6 +1432,13 @@ private void setupsChanged (final Map newSetups, final List(cookies), modelDataList); @@ -1409,6 +1453,7 @@ private final class RefreshNodesTask implements Runnable, Cancellable { @Override public void run() { + final long generation = refreshGeneration.get(); canceled = false; final List nodes = new LinkedList(); final Map localSetups; @@ -1421,10 +1466,11 @@ public void run() { } else { localSetups = getRevisionToRevisionSetups(nodes); } - if (canceled) { + if (canceled || !isCurrent(generation)) { + closeSetups(localSetups); return; } - setupsChanged(localSetups, nodes); + setupsChanged(localSetups, nodes, generation); } @Override @@ -1552,6 +1598,7 @@ private final class ApplyChangesTask implements Runnable, Cancellable { @Override public void run() { + final long generation = refreshGeneration.get(); canceled = false; final Set events; synchronized (changes) { @@ -1637,6 +1684,10 @@ public Map run() { Mutex.EVENT.readAccess(new Runnable() { @Override public void run() { + if (canceled || !isCurrent(generation)) { + closeSetups(localSetups); + return; + } for (DiffNode node : toAdd) { setups.put(node.getFile(), localSetups.get(node.getFile())); EditorCookie cookie = cookies.get(node.getFile()); @@ -1705,6 +1756,9 @@ public void run () { EventQueue.invokeLater(new Runnable() { @Override public void run () { + if (closed) { + return; + } panel.cmbDiffTreeFirst.setModel(new DefaultComboBoxModel(modelRight.toArray(new Object[0]))); panel.cmbDiffTreeSecond.setModel(new DefaultComboBoxModel(modelLeft.toArray(new Object[0]))); panel.cmbDiffTreeFirst.setEnabled(true); @@ -1719,28 +1773,32 @@ private class DiffPrepareTask implements Runnable, Cancellable { private final Setup[] prepareSetups; private Setup selectedSetup; private volatile boolean canceled; + private final long generation; - public DiffPrepareTask(Setup[] prepareSetups) { + public DiffPrepareTask(Setup[] prepareSetups, long generation) { assert EventQueue.isDispatchThread(); assert !Arrays.asList(prepareSetups).contains(null); this.prepareSetups = prepareSetups; this.selectedSetup = prepareSetups[0]; + this.generation = generation; } @Override public void run() { - canceled = false; + if (canceled || !isCurrent(generation)) { + return; + } IOException exception = null; DiffNode[] neighbourNodes = prepareSetupsToRefresh(); for (DiffNode n : neighbourNodes) { final Setup s = n.getSetup(); - if (canceled) return; + if (canceled || !isCurrent(generation)) return; if (s.getView() != null) { continue; } try { s.initSources(); // slow network I/O - if (canceled) { + if (canceled || !isCurrent(generation)) { return; } StreamSource ss1 = s.getFirstSource(); @@ -1749,14 +1807,17 @@ public void run() { requestedLeftLine = getMatchingLine(ss2, ss1, requestedRightLine); } final DiffController view = DiffController.createEnhanced(ss1, ss2); // possibly executing slow external diff - view.addPropertyChangeListener(MultiDiffPanelController.this); - if (canceled) { + if (canceled || !isCurrent(generation)) { return; } - s.setView(view); EventQueue.invokeLater(new Runnable() { @Override public void run() { + if (canceled || !isCurrent(generation)) { + return; + } + view.addPropertyChangeListener(MultiDiffPanelController.this); + s.setView(view); if (s.equals(currentSetup)) { setDiffIndex(s, 0, false); } diff --git a/ide/git/test/unit/src/org/netbeans/modules/git/ui/diff/DiffLifecycleTest.java b/ide/git/test/unit/src/org/netbeans/modules/git/ui/diff/DiffLifecycleTest.java new file mode 100644 index 000000000000..27ded2c673cd --- /dev/null +++ b/ide/git/test/unit/src/org/netbeans/modules/git/ui/diff/DiffLifecycleTest.java @@ -0,0 +1,254 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file distributed + * with this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.netbeans.modules.git.ui.diff; + +import java.awt.AWTEvent; +import java.awt.EventQueue; +import java.awt.Toolkit; +import java.awt.event.InvocationEvent; +import java.io.File; +import java.io.IOException; +import java.lang.reflect.Field; +import java.lang.reflect.InvocationTargetException; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import javax.swing.JComponent; +import javax.swing.JPanel; +import org.netbeans.api.diff.StreamSource; +import org.netbeans.junit.MockServices; +import org.netbeans.modules.git.AbstractGitTestCase; +import org.netbeans.modules.git.FileInformation; +import org.netbeans.modules.git.GitVCS; +import org.netbeans.modules.git.GitModuleConfig; +import org.netbeans.modules.git.ui.repository.Revision; +import org.netbeans.modules.versioning.spi.VCSContext; +import org.netbeans.spi.diff.DiffControllerImpl; +import org.netbeans.spi.diff.DiffControllerProvider; +import org.openide.nodes.AbstractNode; +import org.openide.nodes.Children; +import org.openide.nodes.Node; +import org.openide.util.RequestProcessor; +import org.openide.util.lookup.Lookups; + +/** Real-controller ordering regressions, with gates rather than sleeps/polling. */ +public class DiffLifecycleTest extends AbstractGitTestCase { + private MultiDiffPanelController controller; + private RequestProcessor rp; + private PublicationQueue publicationQueue; + private boolean controllerClosed; + private FileInformation.Mode previousMode; + private final CountDownLatch releaseWorker = new CountDownLatch(1); + private final CountDownLatch releaseReplacement = new CountDownLatch(1); + + public DiffLifecycleTest(String name) { super(name); } + + @Override protected void setUp() throws Exception { + super.setUp(); + MockServices.setServices(GitVCS.class, ControlledDiffProvider.class); + previousMode = GitModuleConfig.getDefault().getLastUsedModificationContext(); + GitModuleConfig.getDefault().setLastUsedModificationContext(FileInformation.Mode.HEAD_VS_WORKING_TREE); + ControlledDiffProvider.entered = null; + ControlledDiffProvider.release = null; + rp = (RequestProcessor) field(MultiDiffPanelController.class, "RP").get(null); + drainWorker(); + } + + @Override protected void tearDown() throws Exception { + releaseWorker.countDown(); + releaseReplacement.countDown(); + if (ControlledDiffProvider.release != null) ControlledDiffProvider.release.countDown(); + try { + onEdt(() -> { + closeController(); + if (publicationQueue != null) publicationQueue.remove(); + }); + drainWorker(); + onEdt(() -> {}); + } finally { + GitModuleConfig.getDefault().setLastUsedModificationContext(previousMode); + super.tearDown(); + } + } + + private File unstagedFile() throws Exception { + File file = new File(repositoryLocation, "unstaged.txt"); + write(file, "committed\n"); + add(file); commit(file); + write(file, "working tree\n"); + getCache().refreshAllRoots(new File[] {file}); + FileInformation status = getCache().getStatus(file); + assertTrue(status.containsStatus(FileInformation.Status.MODIFIED_HEAD_WORKING_TREE)); + assertFalse(status.containsStatus(FileInformation.Status.MODIFIED_HEAD_INDEX)); + return file; + } + + private void preparePausedRefresh(File file) throws Exception { + CountDownLatch occupied = new CountDownLatch(1); + rp.post(() -> { + occupied.countDown(); + await(releaseWorker); + }); + await(occupied); + onEdt(() -> { + controller = new MultiDiffPanelController(VCSContext.forNodes(new Node[] { + new AbstractNode(Children.LEAF, Lookups.fixed(file))}), Revision.HEAD, Revision.LOCAL); + publicationQueue = new PublicationQueue(rp); + Toolkit.getDefaultToolkit().getSystemEventQueue().push(publicationQueue); + }); + } + + public void testRefreshCannotPublishAfterWindowClosed() throws Exception { + preparePausedRefresh(unstagedFile()); + onEdt(() -> { + releaseWorker.countDown(); + await(publicationQueue.posted); + // The background refresh has passed its cancellation check and + // queued its publication. Close before the EDT executes it. + closeController(); + }); + drainWorker(); + onEdt(() -> assertEquals("closed window was repopulated by a stale refresh", 0, nodeCount())); + } + + public void testOldModeRefreshCannotPublishIntoNewMode() throws Exception { + preparePausedRefresh(unstagedFile()); + CountDownLatch replacementPaused = new CountDownLatch(1); + onEdt(() -> { + releaseWorker.countDown(); + await(publicationQueue.posted); + // Run after the old publication, before the replacement refresh. + rp.post(() -> { replacementPaused.countDown(); await(releaseReplacement); }); + panel().tgbHeadVsIndex.doClick(0); + }); + await(replacementPaused); + onEdt(() -> { + assertTrue(panel().tgbHeadVsIndex.isSelected()); + assertEquals("an obsolete working-tree row appeared in HEAD/index mode", 0, nodeCount()); + }); + } + + public void testClosingWindowCancelsRunningDiffPreparationWorker() throws Exception { + File file = unstagedFile(); + ControlledDiffProvider.entered = new CountDownLatch(1); + ControlledDiffProvider.release = new CountDownLatch(1); + final Setup[] selected = new Setup[1]; + final Object[] preparation = new Object[1]; + onEdt(() -> { + controller = new MultiDiffPanelController(file, Revision.HEAD, Revision.LOCAL, -1); + try { + selected[0] = (Setup) field(MultiDiffPanelController.class, "currentSetup").get(controller); + preparation[0] = field(MultiDiffPanelController.class, "dpt").get(controller); + } + catch (ReflectiveOperationException error) { throw new AssertionError(error); } + }); + await(ControlledDiffProvider.entered); + onEdt(this::closeController); + ControlledDiffProvider.release.countDown(); + drainWorker(); + onEdt(() -> assertNull("cancelled preparation installed a diff after close", selected[0].getView())); + assertTrue("the preparation worker's cancellation flag was not set", + field(preparation[0].getClass(), "canceled").getBoolean(preparation[0])); + } + + private void closeController() { + if (controller != null && !controllerClosed) { + controller.componentClosed(); + controllerClosed = true; + } + } + + private static void onEdt(Runnable runnable) throws Exception { + try { + EventQueue.invokeAndWait(runnable); + } catch (InvocationTargetException error) { + if (error.getCause() instanceof Error cause) throw cause; + if (error.getCause() instanceof RuntimeException cause) throw cause; + throw error; + } + } + + private MultiDiffPanel panel() { + return (MultiDiffPanel) controller.getPanel(); + } + + private int nodeCount() { + try { + return ((DiffFileTable) field(MultiDiffPanelController.class, "fileListComponent").get(controller)).getNodes().size(); + } catch (ReflectiveOperationException error) { throw new AssertionError(error); } + } + + private static Field field(Class type, String name) throws NoSuchFieldException { + Field field = type.getDeclaredField(name); + field.setAccessible(true); + return field; + } + + private void drainWorker() throws InterruptedException { + assertTrue("diff worker did not drain", rp.post(() -> {}).waitFinished(15000)); + } + + private static void await(CountDownLatch latch) { + try { + if (!latch.await(15, TimeUnit.SECONDS)) throw new AssertionError("test gate timed out"); + } catch (InterruptedException error) { + Thread.currentThread().interrupt(); + throw new AssertionError(error); + } + } + + /** Observes the real controller's EDT handoff; does not replace its code. */ + private static final class PublicationQueue extends EventQueue { + final CountDownLatch posted = new CountDownLatch(1); + final RequestProcessor rp; + + PublicationQueue(RequestProcessor rp) { this.rp = rp; } + + @Override public void postEvent(AWTEvent event) { + super.postEvent(event); + if (event instanceof InvocationEvent && rp.isRequestProcessorThread()) { + StackTraceElement[] stack = Thread.currentThread().getStackTrace(); + for (int i = 1; i < stack.length; i++) { + StackTraceElement frame = stack[i]; + if (frame.getClassName().equals(MultiDiffPanelController.class.getName()) + && frame.getMethodName().equals("setupsChanged") + && stack[i - 1].getClassName().equals("org.openide.util.Mutex") + && stack[i - 1].getMethodName().equals("readAccess")) { + posted.countDown(); + break; + } + } + } + } + + void remove() { pop(); } + } + + public static final class ControlledDiffProvider extends DiffControllerProvider { + static volatile CountDownLatch entered; + static volatile CountDownLatch release; + + @Override public DiffControllerImpl createDiffController(StreamSource base, StreamSource modified) throws IOException { + if (entered != null) { + entered.countDown(); + await(release); + } + return new DiffControllerImpl() { + private final JPanel component = new JPanel(); + @Override public JComponent getJComponent() { return component; } + @Override public int getDifferenceCount() { return 0; } + }; + } + } +}