diff --git a/bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java b/bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java index 7d46050af1f..59db5e76128 100644 --- a/bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java +++ b/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> fElementsToMatches; private final List fListeners; - private final AtomicInteger matchCount; + private final AtomicLong modCount= new AtomicLong(); + private final AtomicReference cachedMatchCount= new AtomicReference<>(); + private final ConcurrentHashMap 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 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 removedMatches = new ArrayList<>(); - for (Object object : elements) { - Set matches = fElementsToMatches.remove(object); - if (matches != null) { - removedMatches.addAll(matches); + try { + for (Object object : elements) { + Set 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) { + } } diff --git a/tests/org.eclipse.search.tests/src/org/eclipse/search/tests/AbstractTextSearchResultMatchCountTest.java b/tests/org.eclipse.search.tests/src/org/eclipse/search/tests/AbstractTextSearchResultMatchCountTest.java new file mode 100644 index 00000000000..f1a15e436c8 --- /dev/null +++ b/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()); + } +} diff --git a/tests/org.eclipse.search.tests/src/org/eclipse/search/tests/AllSearchTests.java b/tests/org.eclipse.search.tests/src/org/eclipse/search/tests/AllSearchTests.java index 8207fbe01e6..36baa3f11ab 100644 --- a/tests/org.eclipse.search.tests/src/org/eclipse/search/tests/AllSearchTests.java +++ b/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