Fix stale cached match count in AbstractTextSearchResult

bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java

@@ -24,8 +24,8 @@
 import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.ConcurrentSkipListSet;
 import java.util.concurrent.CopyOnWriteArrayList;
-import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.atomic.AtomicReference;
 
 import org.eclipse.jface.text.Position;
 
@@ -53,7 +53,9 @@ public abstract class AbstractTextSearchResult implements ISearchResult {
 	 */
 	private final ConcurrentMap<Object, ConcurrentSkipListSet<Match>> fElementsToMatches;
 	private final List<ISearchResultListener> fListeners;
-	private final AtomicInteger matchCount;
+	private final AtomicLong modCount= new AtomicLong();
+	private final AtomicReference<CountedMatches> cachedMatchCount= new AtomicReference<>();
+
 	private final ConcurrentHashMap<Match, Long> collisionOrder;
 	private final AtomicLong collisionCounter;
 
@@ -65,7 +67,6 @@ public abstract class AbstractTextSearchResult implements ISearchResult {
 	protected AbstractTextSearchResult() {
 		fElementsToMatches= new ConcurrentHashMap<>();
 		fListeners = new CopyOnWriteArrayList<>();
-		matchCount = new AtomicInteger(0);
 		fMatchFilters= null; // filtering disabled by default
 		collisionOrder = new ConcurrentHashMap<>();
 		collisionCounter = new AtomicLong(0);
@@ -205,10 +206,13 @@ public void updateMatch(Match match, Position pos) {
 	 *         already present
 	 */
 	private boolean didAddMatch(Match match) {
-		matchCount.set(0);
 		updateFilterState(match);
-		return fElementsToMatches.computeIfAbsent(match.getElement(),
-				k -> new ConcurrentSkipListSet<>(this::compare)).add(match);
+		try {
+			return fElementsToMatches.computeIfAbsent(match.getElement(),
+					k -> new ConcurrentSkipListSet<>(this::compare)).add(match);
+		} finally {
+			modCount.incrementAndGet();
+		}
 	}
 
 	/**
@@ -266,9 +270,9 @@ public void removeAll() {
 		fireChange(new RemoveAllEvent(this));
 	}
 	private void doRemoveAll() {
-		matchCount.set(0);
 		fElementsToMatches.clear();
 		collisionOrder.clear();
+		modCount.incrementAndGet();
 	}
 
 	/**
@@ -308,7 +312,6 @@ public void removeMatches(Match[] matches) {
 
 
 	private boolean didRemoveMatch(Match match) {
-		matchCount.set(0);
 		boolean[] existed = new boolean[1];
 		fElementsToMatches.computeIfPresent(match.getElement(), (f, matches) -> {
 			existed[0] = matches.remove(match);
@@ -320,6 +323,7 @@ private boolean didRemoveMatch(Match match) {
 		if (existed[0]) {
 			collisionOrder.remove(match);
 		}
+		modCount.incrementAndGet();
 		return existed[0];
 	}
 
@@ -390,21 +394,18 @@ private boolean updateFilterState(Match match) {
 	 * @return total number of matches
 	 */
 	public int getMatchCount() {
-		final int oldCount = matchCount.get();
-		if (oldCount != 0) {
-			return oldCount;
+		// Read the stamp before summing so a concurrent modification invalidates the value computed below
+		long stamp = modCount.get();
+		CountedMatches cached = cachedMatchCount.get();
+		if (cached != null && cached.stamp() == stamp) {
+			return cached.count();
 		}
-		// The oldCount is zero here => we have to calculate again
 		int newCount = 0;
 		for (Set<Match> element : fElementsToMatches.values()) {
 			newCount += element.size();
 		}
-		if (matchCount.compareAndSet(0, newCount)) {
-			// Only return if not changed meanwhile
-			return newCount;
-		}
-		// Changed once again, fetch again latest value
-		return getMatchCount();
+		cachedMatchCount.set(new CountedMatches(stamp, newCount));
+		return newCount;
 	}
 
 	/**
@@ -557,13 +558,16 @@ public MatchFilter[] getAllMatchFilters() {
 	 * @since 3.19
 	 */
 	public void removeElements(Collection<?> elements) {
-		matchCount.set(0);
 		List<Match> removedMatches = new ArrayList<>();
-		for (Object object : elements) {
-			Set<Match> matches = fElementsToMatches.remove(object);
-			if (matches != null) {
-				removedMatches.addAll(matches);
+		try {
+			for (Object object : elements) {
+				Set<Match> matches = fElementsToMatches.remove(object);
+				if (matches != null) {
+					removedMatches.addAll(matches);
+				}
 			}
+		} finally {
+			modCount.incrementAndGet();
 		}
 		if (!removedMatches.isEmpty()) {
 			if (!collisionOrder.isEmpty()) {
@@ -572,4 +576,8 @@ public void removeElements(Collection<?> elements) {
 			fireChange(getSearchResultEvent(removedMatches, MatchEvent.REMOVED));
 		}
 	}
+
+	/** Match count tagged with the modification stamp it was computed under. */
+	private record CountedMatches(long stamp, int count) {
+	}
 }

tests/org.eclipse.search.tests/src/org/eclipse/search/tests/AbstractTextSearchResultMatchCountTest.java

@@ -0,0 +1,106 @@
+/*******************************************************************************
+ * Copyright (c) 2026 vogella GmbH and others.
+ *
+ * This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License 2.0
+ * which accompanies this distribution, and is available at
+ * https://www.eclipse.org/legal/epl-2.0/
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ *******************************************************************************/
+package org.eclipse.search.tests;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
+import org.junit.jupiter.api.Test;
+
+import org.eclipse.jface.resource.ImageDescriptor;
+
+import org.eclipse.search.ui.ISearchQuery;
+import org.eclipse.search.ui.text.AbstractTextSearchResult;
+import org.eclipse.search.ui.text.IEditorMatchAdapter;
+import org.eclipse.search.ui.text.IFileMatchAdapter;
+import org.eclipse.search.ui.text.Match;
+
+public class AbstractTextSearchResultMatchCountTest {
+
+	private static final class TestResult extends AbstractTextSearchResult {
+		@Override
+		public String getLabel() {
+			return "";
+		}
+
+		@Override
+		public String getTooltip() {
+			return "";
+		}
+
+		@Override
+		public ImageDescriptor getImageDescriptor() {
+			return null;
+		}
+
+		@Override
+		public ISearchQuery getQuery() {
+			return null;
+		}
+
+		@Override
+		public IEditorMatchAdapter getEditorMatchAdapter() {
+			return null;
+		}
+
+		@Override
+		public IFileMatchAdapter getFileMatchAdapter() {
+			return null;
+		}
+	}
+
+	/** Element whose next hashCode() call parks the calling thread until released. */
+	private static final class BlockingElement {
+		final CountDownLatch entered = new CountDownLatch(1);
+		final CountDownLatch release = new CountDownLatch(1);
+		volatile boolean armed = true;
+
+		@Override
+		public int hashCode() {
+			if (armed) {
+				armed = false;
+				entered.countDown();
+				try {
+					release.await(10, TimeUnit.SECONDS);
+				} catch (InterruptedException e) {
+					Thread.currentThread().interrupt();
+				}
+			}
+			return 42;
+		}
+	}
+
+	@Test
+	public void testMatchCountIsNotStaleWhenReadWhileMatchIsAdded() throws Exception {
+		TestResult result = new TestResult();
+		result.addMatch(new Match("element", 0, 5));
+		result.addMatch(new Match("element", 10, 5));
+
+		BlockingElement blocking = new BlockingElement();
+		Thread adder = new Thread(() -> result.addMatch(new Match(blocking, 0, 5)));
+		adder.setDaemon(true);
+		adder.start();
+		try {
+			assertTrue(blocking.entered.await(10, TimeUnit.SECONDS));
+			// Count while the add is in flight, so the count gets cached before the match lands
+			assertEquals(2, result.getMatchCount());
+		} finally {
+			blocking.release.countDown();
+			adder.join(10_000);
+		}
+		assertFalse(adder.isAlive());
+		assertEquals(3, result.getMatchCount());
+	}
+}

tests/org.eclipse.search.tests/src/org/eclipse/search/tests/AllSearchTests.java

@@ -26,7 +26,8 @@
 		TextSearchRegistryTest.class,
 		GlobalNextPrevSearchEntryHandlerTest.class,
 		GlobalNextPrevSearchEntryHandlerIntegrationTest.class,
-		SelectAllActionTest.class
+		SelectAllActionTest.class,
+		AbstractTextSearchResultMatchCountTest.class
 })
 public class AllSearchTests {
 	// see @SelectClasses