From 95ccf7172dd88f87c5e4ea20272ab015c276e318 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Dinis=20Ferreira?= Date: Sat, 26 Sep 2026 02:14:04 +0200 Subject: [PATCH] fix(builder): abandon the load operation when a resource load times out ParallelLoadOperation.next() decremented the outstanding-result counter even when poll() timed out. From then on the counter was one short of the results still coming, so the builder either aborted the cluster as if it had been cancelled or, in writeResources, left some other resource silently unindexed. A longer timeout makes this rarer but cannot prevent it. Decrement only when a result is delivered. A timeout cannot be attributed to a URI, and a load that never finishes would otherwise keep the builder waiting forever, so a timeout now cancels the whole operation and says so. writeResources treats that like the linking phase already did on master: it cancels the build instead of dropping the remaining resources, so the next build is a full build rather than one with a stale index. The check is a public predicate, isAbandonedByTimeout, so that it can be tested. Co-Authored-By: Claude Opus 5.5 --- .../META-INF/MANIFEST.MF | 2 +- com.avaloq.tools.ddk.xtext.builder/pom.xml | 2 +- .../MonitoredClusteringBuilderState.java | 18 +++ .../ParallelResourceLoader.java | 15 +- .../tools/ddk/xtext/XtextTestSuite.java | 4 + .../xtext/builder/BuilderLoadTimeoutTest.java | 63 ++++++++ .../ParallelResourceLoaderTest.java | 151 ++++++++++++++++++ 7 files changed, 251 insertions(+), 4 deletions(-) create mode 100644 com.avaloq.tools.ddk.xtext.test/src/com/avaloq/tools/ddk/xtext/builder/BuilderLoadTimeoutTest.java create mode 100644 com.avaloq.tools.ddk.xtext.test/src/com/avaloq/tools/ddk/xtext/builder/resourceloader/ParallelResourceLoaderTest.java diff --git a/com.avaloq.tools.ddk.xtext.builder/META-INF/MANIFEST.MF b/com.avaloq.tools.ddk.xtext.builder/META-INF/MANIFEST.MF index ce944bfc61..7447b5500e 100644 --- a/com.avaloq.tools.ddk.xtext.builder/META-INF/MANIFEST.MF +++ b/com.avaloq.tools.ddk.xtext.builder/META-INF/MANIFEST.MF @@ -2,7 +2,7 @@ Manifest-Version: 1.0 Bundle-ManifestVersion: 2 Bundle-Name: com.avaloq.tools.ddk.xtext.builder Bundle-SymbolicName: com.avaloq.tools.ddk.xtext.builder;singleton:=true -Bundle-Version: 17.3.1.qualifier +Bundle-Version: 17.3.2.qualifier Bundle-Vendor: Avaloq Group AG Require-Bundle: org.eclipse.xtext.builder, org.eclipse.xtext.ui, diff --git a/com.avaloq.tools.ddk.xtext.builder/pom.xml b/com.avaloq.tools.ddk.xtext.builder/pom.xml index e5d12cee19..710f52ccbc 100644 --- a/com.avaloq.tools.ddk.xtext.builder/pom.xml +++ b/com.avaloq.tools.ddk.xtext.builder/pom.xml @@ -6,7 +6,7 @@ 18.0.1-SNAPSHOT ../ddk-parent - 17.3.1-SNAPSHOT + 17.3.2-SNAPSHOT com.avaloq.tools.ddk com.avaloq.tools.ddk.xtext.builder eclipse-plugin diff --git a/com.avaloq.tools.ddk.xtext.builder/src/com/avaloq/tools/ddk/xtext/builder/MonitoredClusteringBuilderState.java b/com.avaloq.tools.ddk.xtext.builder/src/com/avaloq/tools/ddk/xtext/builder/MonitoredClusteringBuilderState.java index e056df3fe4..47c20363a3 100644 --- a/com.avaloq.tools.ddk.xtext.builder/src/com/avaloq/tools/ddk/xtext/builder/MonitoredClusteringBuilderState.java +++ b/com.avaloq.tools.ddk.xtext.builder/src/com/avaloq/tools/ddk/xtext/builder/MonitoredClusteringBuilderState.java @@ -1101,6 +1101,11 @@ private List writeResources(final Collection toWrite, final BuildData } } catch (final WrappedException ex) { pollForCancellation(monitor); + if (isAbandonedByTimeout(ex, loadOperation)) { + // the remaining resources must not silently stay unindexed + LOGGER.warn(ex.getCause().getMessage()); + throw new OperationCanceledException(); // NOPMD PreserveStackTrace - the timeout is logged above + } if (uri == null && ex instanceof LoadOperationException) { uri = ((LoadOperationException) ex).getUri(); } @@ -1330,6 +1335,19 @@ protected void queueAffectedResources(final Set allRemainingURIs, final IRe } } + /** + * Whether a load failure means the load operation timed out and abandoned all resources it had not delivered yet. + * + * @param exception + * the exception thrown by {@link IResourceLoader.LoadOperation#next()}, must not be {@code null} + * @param loadOperation + * the load operation that threw it, must not be {@code null} + * @return {@code true} if the operation was abandoned after a timeout + */ + public static boolean isAbandonedByTimeout(final WrappedException exception, final IResourceLoader.LoadOperation loadOperation) { + return exception instanceof LoadOperationException && exception.getCause() instanceof TimeoutException && !loadOperation.hasNext(); + } + /** * Checks if the given {@link IProgressMonitor} was cancelled. *

diff --git a/com.avaloq.tools.ddk.xtext.builder/src/com/avaloq/tools/ddk/xtext/builder/resourceloader/ParallelResourceLoader.java b/com.avaloq.tools.ddk.xtext.builder/src/com/avaloq/tools/ddk/xtext/builder/resourceloader/ParallelResourceLoader.java index 26e570a79a..50f6ada2e9 100644 --- a/com.avaloq.tools.ddk.xtext.builder/src/com/avaloq/tools/ddk/xtext/builder/resourceloader/ParallelResourceLoader.java +++ b/com.avaloq.tools.ddk.xtext.builder/src/com/avaloq/tools/ddk/xtext/builder/resourceloader/ParallelResourceLoader.java @@ -216,9 +216,14 @@ public LoadResult next() { throw new NoSuchElementException("The resource queue is empty or the execution was cancelled."); //$NON-NLS-1$ } Triple result = null; + boolean timedOut = false; try { result = resourceQueue.poll(waitTime, TimeUnit.MILLISECONDS); - toProcess--; + if (result != null) { + toProcess--; + } else { + timedOut = true; + } } catch (InterruptedException e) { Thread.currentThread().interrupt(); } @@ -227,7 +232,13 @@ public LoadResult next() { synchronized (currentlyProcessedUris) { currentUris = Joiner.on(", ").join(currentlyProcessedUris); //$NON-NLS-1$ } - throw new LoadOperationException(null, new TimeoutException(String.format("Resource load job didn't return a result after %d ms. Resources being currently loaded: %s", waitTime, currentUris))); //$NON-NLS-1$ + String message = String.format("Resource load job didn't return a result after %d ms. Resources being currently loaded: %s", waitTime, currentUris); //$NON-NLS-1$ + if (timedOut) { + // a timeout cannot be attributed to a URI and a hung load would never finish, so abandon the whole operation + cancel(); + message += " The remaining loads are abandoned."; //$NON-NLS-1$ + } + throw new LoadOperationException(null, new TimeoutException(message)); } URI uri = result.getFirst(); diff --git a/com.avaloq.tools.ddk.xtext.test/src/com/avaloq/tools/ddk/xtext/XtextTestSuite.java b/com.avaloq.tools.ddk.xtext.test/src/com/avaloq/tools/ddk/xtext/XtextTestSuite.java index 2a6b079e5d..e563412993 100644 --- a/com.avaloq.tools.ddk.xtext.test/src/com/avaloq/tools/ddk/xtext/XtextTestSuite.java +++ b/com.avaloq.tools.ddk.xtext.test/src/com/avaloq/tools/ddk/xtext/XtextTestSuite.java @@ -13,7 +13,9 @@ import org.junit.platform.suite.api.SelectClasses; import org.junit.platform.suite.api.Suite; +import com.avaloq.tools.ddk.xtext.builder.BuilderLoadTimeoutTest; import com.avaloq.tools.ddk.xtext.builder.XtextBuildTriggerTest; +import com.avaloq.tools.ddk.xtext.builder.resourceloader.ParallelResourceLoaderTest; import com.avaloq.tools.ddk.xtext.jupiter.formatter.FormatterTest; import com.avaloq.tools.ddk.xtext.linking.AbstractFragmentProviderTest; import com.avaloq.tools.ddk.xtext.linking.ShortFragmentProviderTest; @@ -36,6 +38,8 @@ AbstractSelectorFragmentProviderTest.class, ResourceDescriptionDeltaTest.class, XtextBuildTriggerTest.class, + BuilderLoadTimeoutTest.class, + ParallelResourceLoaderTest.class, FormatterTest.class, QualifiedNamePatternTest.class, BugAig1084.class, diff --git a/com.avaloq.tools.ddk.xtext.test/src/com/avaloq/tools/ddk/xtext/builder/BuilderLoadTimeoutTest.java b/com.avaloq.tools.ddk.xtext.test/src/com/avaloq/tools/ddk/xtext/builder/BuilderLoadTimeoutTest.java new file mode 100644 index 0000000000..8cacbdab11 --- /dev/null +++ b/com.avaloq.tools.ddk.xtext.test/src/com/avaloq/tools/ddk/xtext/builder/BuilderLoadTimeoutTest.java @@ -0,0 +1,63 @@ +/******************************************************************************* + * Copyright (c) 2026 Avaloq Group AG and others. + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License v1.0 + * which accompanies this distribution, and is available at + * http://www.eclipse.org/legal/epl-v10.html + * + * Contributors: + * Avaloq Group AG - initial API and implementation + *******************************************************************************/ +package com.avaloq.tools.ddk.xtext.builder; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.io.IOException; +import java.util.concurrent.TimeoutException; + +import org.eclipse.emf.common.util.URI; +import org.eclipse.emf.common.util.WrappedException; +import org.eclipse.xtext.builder.resourceloader.IResourceLoader.LoadOperation; +import org.eclipse.xtext.builder.resourceloader.IResourceLoader.LoadOperationException; +import org.junit.jupiter.api.Test; + + +/** + * Tests for {@link MonitoredClusteringBuilderState#isAbandonedByTimeout(WrappedException, LoadOperation)}. + */ +@SuppressWarnings("nls") +public class BuilderLoadTimeoutTest { + + private static final TimeoutException TIMEOUT = new TimeoutException("no result"); + + @Test + public void timeoutWithNothingLeftIsAbandonment() { + assertTrue(MonitoredClusteringBuilderState.isAbandonedByTimeout(new LoadOperationException(null, TIMEOUT), operation(false))); + } + + @Test + public void timeoutWithLoadsLeftIsNotAbandonment() { + assertFalse(MonitoredClusteringBuilderState.isAbandonedByTimeout(new LoadOperationException(null, TIMEOUT), operation(true))); + } + + @Test + public void loadFailureIsNotAbandonment() { + LoadOperationException failure = new LoadOperationException(URI.createURI("platform:/resource/project/a.test"), new IOException("cannot read")); + assertFalse(MonitoredClusteringBuilderState.isAbandonedByTimeout(failure, operation(false))); + } + + @Test + public void otherWrappedExceptionIsNotAbandonment() { + assertFalse(MonitoredClusteringBuilderState.isAbandonedByTimeout(new WrappedException(TIMEOUT), operation(false))); + } + + private static LoadOperation operation(final boolean hasNext) { + LoadOperation operation = mock(LoadOperation.class); + when(operation.hasNext()).thenReturn(hasNext); + return operation; + } + +} diff --git a/com.avaloq.tools.ddk.xtext.test/src/com/avaloq/tools/ddk/xtext/builder/resourceloader/ParallelResourceLoaderTest.java b/com.avaloq.tools.ddk.xtext.test/src/com/avaloq/tools/ddk/xtext/builder/resourceloader/ParallelResourceLoaderTest.java new file mode 100644 index 0000000000..3d763c0766 --- /dev/null +++ b/com.avaloq.tools.ddk.xtext.test/src/com/avaloq/tools/ddk/xtext/builder/resourceloader/ParallelResourceLoaderTest.java @@ -0,0 +1,151 @@ +/******************************************************************************* + * Copyright (c) 2026 Avaloq Group AG and others. + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License v1.0 + * which accompanies this distribution, and is available at + * http://www.eclipse.org/legal/epl-v10.html + * + * Contributors: + * Avaloq Group AG - initial API and implementation + *******************************************************************************/ +package com.avaloq.tools.ddk.xtext.builder.resourceloader; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.io.IOException; +import java.util.Collections; +import java.util.List; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.function.Function; + +import org.eclipse.emf.common.util.URI; +import org.eclipse.emf.common.util.WrappedException; +import org.eclipse.emf.ecore.resource.Resource; +import org.eclipse.emf.ecore.resource.ResourceSet; +import org.eclipse.emf.ecore.resource.impl.ResourceImpl; +import org.eclipse.emf.ecore.resource.impl.ResourceSetImpl; +import org.eclipse.xtext.builder.resourceloader.IResourceLoader.LoadOperation; +import org.eclipse.xtext.builder.resourceloader.IResourceLoader.LoadOperationException; +import org.eclipse.xtext.builder.resourceloader.IResourceLoader.Sorter; +import org.eclipse.xtext.resource.IResourceServiceProvider; +import org.eclipse.xtext.resource.persistence.SourceLevelURIsAdapter; +import org.eclipse.xtext.ui.resource.IResourceSetProvider; +import org.junit.jupiter.api.Test; + +import com.google.common.util.concurrent.Uninterruptibles; +import com.google.inject.Guice; + + +/** + * Tests for {@link ParallelResourceLoader}. + */ +@SuppressWarnings("nls") +public class ParallelResourceLoaderTest { + + private static final URI SLOW_URI = URI.createURI("platform:/resource/project/slow.test"); + private static final URI OTHER_URI = URI.createURI("platform:/resource/project/other.test"); + private static final long TIMEOUT_MILLIS = 50; + private static final long GENEROUS_TIMEOUT_MILLIS = TimeUnit.SECONDS.toMillis(10); + private static final long SETTLE_MILLIS = 200; + /** Unbounded result queue, and the synchronous hand-off used in production. */ + private static final int[] QUEUE_SIZES = {-1, 0}; + + @Test + public void timeoutAbandonsEveryOutstandingLoad() { + for (int queueSize : QUEUE_SIZES) { + CountDownLatch loadReleased = new CountDownLatch(1); + Set loaded = ConcurrentHashMap.newKeySet(); + LoadOperation operation = load(List.of(SLOW_URI, OTHER_URI), queueSize, TIMEOUT_MILLIS, uri -> { + loaded.add(uri); + Uninterruptibles.awaitUninterruptibly(loadReleased); + return new ResourceImpl(uri); + }); + try { + assertTimesOut(operation); + assertFalse(operation.hasNext(), "one timeout must abandon all outstanding loads, not just one"); + + loadReleased.countDown(); + Uninterruptibles.sleepUninterruptibly(SETTLE_MILLIS, TimeUnit.MILLISECONDS); + assertFalse(loaded.contains(OTHER_URI), "queued loads must not start after the operation was abandoned"); + } finally { + loadReleased.countDown(); + operation.cancel(); + } + } + } + + @Test + public void failedLoadDoesNotAbandonTheOperation() { + for (int queueSize : QUEUE_SIZES) { + LoadOperation operation = load(List.of(SLOW_URI, OTHER_URI), queueSize, GENEROUS_TIMEOUT_MILLIS, uri -> { + if (SLOW_URI.equals(uri)) { + throw new WrappedException(new IOException("cannot read")); + } + return new ResourceImpl(uri); + }); + try { + LoadOperationException failure = assertThrows(LoadOperationException.class, operation::next); + assertSame(SLOW_URI, failure.getUri()); + assertInstanceOf(IOException.class, failure.getCause()); + assertTrue(operation.hasNext(), "a failed load must not abandon the remaining loads"); + assertSame(OTHER_URI, operation.next().getResource().getURI()); + } finally { + operation.cancel(); + } + } + } + + @Test + public void resultWithinTimeoutIsDelivered() { + for (int queueSize : QUEUE_SIZES) { + LoadOperation operation = load(List.of(SLOW_URI), queueSize, GENEROUS_TIMEOUT_MILLIS, ResourceImpl::new); + try { + assertSame(SLOW_URI, operation.next().getResource().getURI()); + assertFalse(operation.hasNext(), "the single result has been delivered"); + } finally { + operation.cancel(); + } + } + } + + private static LoadOperation load(final List uris, final int queueSize, final long timeoutMillis, final Function loadFunction) { + ParallelResourceLoader loader = new ParallelResourceLoader(resourceSetProvider(), new Sorter.NoSorting(), 1, queueSize) { + @Override + protected Resource loadResource(final URI uri, final ResourceSet localResourceSet, final ResourceSet parentResourceSet) { + return loadFunction.apply(uri); + } + }; + IResourceServiceProvider.Registry registry = mock(IResourceServiceProvider.Registry.class); + Guice.createInjector(binder -> binder.bind(IResourceServiceProvider.Registry.class).toInstance(registry)).injectMembers(loader); + loader.setTimeout(timeoutMillis, TimeUnit.MILLISECONDS); + + ResourceSet parent = new ResourceSetImpl(); + SourceLevelURIsAdapter.setSourceLevelUris(parent, Collections.emptySet()); + LoadOperation operation = loader.create(parent, null); + operation.load(uris); + return operation; + } + + private static void assertTimesOut(final LoadOperation operation) { + LoadOperationException timeout = assertThrows(LoadOperationException.class, operation::next); + assertInstanceOf(TimeoutException.class, timeout.getCause()); + } + + private static IResourceSetProvider resourceSetProvider() { + IResourceSetProvider provider = mock(IResourceSetProvider.class); + when(provider.get(any())).thenAnswer(invocation -> new ResourceSetImpl()); + return provider; + } + +}