From 1e332d452a02f17ad6ea61f317d345bcdcf6fc3a Mon Sep 17 00:00:00 2001 From: Emilian Bold Date: Sun, 25 Oct 2026 11:01:22 +0200 Subject: [PATCH 1/2] Fix stale Git diff updates after mode changes and close Invalidate obsolete refresh generations at EDT publication, cancel the preparation worker as well as its task wrapper, and prevent late views and listeners from being installed after close. Add latch-gated controller regressions for mode changes, queued refresh publication after close, and in-flight diff preparation cancellation. Verified the three regressions plus existing test193781 against the compiled controller using NetBeans 31 dependency jars on JDK 21. Assisted-by: DeepSeek V4.1 Flash Assisted-by: OpenAI GPT-6.1 Sol --- .../git/ui/diff/MultiDiffPanelController.java | 93 +++++-- .../git/ui/diff/DiffLifecycleTest.java | 254 ++++++++++++++++++ 2 files changed, 331 insertions(+), 16 deletions(-) create mode 100644 ide/git/test/unit/src/org/netbeans/modules/git/ui/diff/DiffLifecycleTest.java 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..f0f7f3620b1b 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 @@ -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(); } @@ -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) { assert EventQueue.isDispatchThread(); assert !Arrays.asList(prepareSetups).contains(null); this.prepareSetups = prepareSetups; this.selectedSetup = prepareSetups[0]; + this.generation = refreshGeneration.get(); } @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; } + }; + } + } +} From e476dfb957bbf61996230c0cdeea1abd540cfc4e Mon Sep 17 00:00:00 2001 From: Emilian Bold Date: Sun, 25 Oct 2026 16:59:34 +0200 Subject: [PATCH 2/2] Pass the diff preparation generation explicitly Capture the generation at the two preparation call sites and pass it into DiffPrepareTask instead of reading outer controller state in its constructor. Leave the reusable refresh and change workers unchanged. Assisted-by: DeepSeek V4.1 Flash Assisted-by: OpenAI GPT-6.1 Sol --- .../modules/git/ui/diff/MultiDiffPanelController.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 f0f7f3620b1b..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 @@ -269,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); } @@ -1320,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); @@ -1775,12 +1775,12 @@ private class DiffPrepareTask implements Runnable, Cancellable { 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 = refreshGeneration.get(); + this.generation = generation; } @Override