Skip to content

feat: introduce timeout and retry parameter for BM executor shutdown #1113

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ava-fred merged 1 commit into from
Apr 11, 2025
Merged

feat: introduce timeout and retry parameter for BM executor shutdown #1113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 75 additions & 22 deletions ...xtext.builder/src/com/avaloq/tools/ddk/xtext/builder/MonitoredClusteringBuilderState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,8 @@ private static final ThreadPoolExecutor makeBinaryStorageExecutor() {
* Unfortunately, we have to duplicate the loading logic here in order to make it better extendable. Parent class lacks a factory method for loading and
* creating the initial ResourceDescriptionsData. We use an {@link IResourceDescriptionsDataProvider} for that.
*/
private volatile boolean isLoaded; // NOPMD; PMD doesn't like "volatile"...
@SuppressWarnings({"PMD.AvoidFieldNameMatchingMethodName", "PMD.AvoidUsingVolatile"})
private volatile boolean isLoaded;

/**
* Don't use a PesistedStateProvider, but use a target platform manager that gives us directly an appropriate ResourceDescriptionsData. The point here is that
Expand Down Expand Up @@ -424,7 +425,8 @@ public synchronized ImmutableList<IResourceDescription.Delta> clean(final Set<UR

@Override
// CHECKSTYLE:CHECK-OFF NestedTryDepth
protected Collection<Delta> doUpdate(final BuildData buildData, final ResourceDescriptionsData newData, final IProgressMonitor monitor) { // NOPMD
@SuppressWarnings("PMD.AvoidInstanceofChecksInCatchClause")
protected Collection<Delta> doUpdate(final BuildData buildData, final ResourceDescriptionsData newData, final IProgressMonitor monitor) {
final SubMonitor progress = SubMonitor.convert(monitor, 100);

// Step 1: Clean the set of deleted URIs. If any of them are also added, they're not deleted.
Expand Down Expand Up @@ -583,7 +585,7 @@ public boolean isCanceled() {
} catch (final Exception ex) {
// CHECKSTYLE:CHECK-ON IllegalCatch
pollForCancellation(monitor);
if (ex instanceof LoadOperationException) { // NOPMD
if (ex instanceof LoadOperationException) {
LoadOperationException loadException = (LoadOperationException) ex;
if (loadException.getCause() instanceof TimeoutException) {
// Load request timed out, URI of the resource is not available
Expand Down Expand Up @@ -778,38 +780,88 @@ protected void deleteBinaryResources(final Set<URI> toBeDeleted) {

/**
* Waits until binary models are stored.
* Uses default parameters for timeout and retries, kept for backward compatibility.
*/
@SuppressFBWarnings("AT_STALE_THREAD_WRITE_OF_PRIMITIVE")
protected void awaitBinaryStorageExecutorTermination() {
LOGGER.info("Waiting for binary resource storage tasks to complete"); //$NON-NLS-1$
awaitBinaryStorageExecutorTermination(1, TimeUnit.MINUTES, 0);
}

/**
* Waits until binary models are stored. Waits for a given time and makes a given number of attempts.
*
* @param timeout
* time to wait for shutdown
* @param unit
* {@link TimeUnit} for timeout
* @param retryCount
* number of retries to attempt
*/
@SuppressWarnings("nls")
@SuppressFBWarnings("AT_STALE_THREAD_WRITE_OF_PRIMITIVE")
protected void awaitBinaryStorageExecutorTermination(final int timeout, final TimeUnit unit, final int retryCount) {
LOGGER.info("Waiting for binary resource storage tasks to complete: timeout {} {}, retryCount {}", timeout, unit, retryCount); //$NON-NLS-1$
if (hwmTimeStamp != null) {
LOGGER.info("high water mark was {} at {}", binaryStorageHighWaterMark, hwmTimeStamp); //$NON-NLS-1$
}

// Stop accepting additional work
binaryStorageExecutor.shutdown();
long queuedTaskCount = binaryStorageExecutor.getQueue().size();
long activeTaskCount = binaryStorageExecutor.getActiveCount();

// Attempt to wait for queued work to complete
int retries = 0;
boolean terminated;
boolean stuck = false;

long prevQueuedTaskCount = binaryStorageExecutor.getQueue().size();
long prevActiveTaskCount = binaryStorageExecutor.getActiveCount();

try {
if (!binaryStorageExecutor.awaitTermination(1, TimeUnit.MINUTES)) {
throw new InterruptedException();
do {
terminated = binaryStorageExecutor.awaitTermination(timeout, unit);
if (!terminated) {
// check whether if there is progress
long currQueuedTaskCount = binaryStorageExecutor.getQueue().size();
long currActiveTaskCount = binaryStorageExecutor.getActiveCount();

if (currQueuedTaskCount < prevQueuedTaskCount || currActiveTaskCount < prevActiveTaskCount) {
LOGGER.warn("Binary resource storage tasks not completed in time, start with {} queued / {} active; now have {} / {}", prevQueuedTaskCount, prevActiveTaskCount, currQueuedTaskCount, currActiveTaskCount);
if (retries < retryCount) {
retries += 1;
LOGGER.warn("retrying shutdown, attempt {} of {}", retries, retryCount);
prevQueuedTaskCount = currQueuedTaskCount;
prevActiveTaskCount = currActiveTaskCount;
}
} else {
LOGGER.warn("Binary resource storage tasks not completed in time, not making progress, stuck on {} / {} queued / active tasks", currQueuedTaskCount, currActiveTaskCount);
stuck = true;
}
}

} while (!terminated && !stuck && retries < retryCount);

if (terminated) {
LOGGER.info("Binary resource storage executor completed.");
} else {
LOGGER.warn("Binary resource storage executor shutdown not successful, terminating");
terminateBinaryStorageExecutor();
}
} catch (InterruptedException e) { // NOPMD ExceptionAsFlowControl
LOGGER.warn(String.format("Binary resource storage tasks not completed in time, start with %d queued / %d active; now have %d / %d", //$NON-NLS-1$
queuedTaskCount, activeTaskCount, binaryStorageExecutor.getQueue().size(), binaryStorageExecutor.getActiveCount()));
binaryStorageExecutor.shutdownNow();
} catch (InterruptedException e) {
LOGGER.warn("Interrupted waiting for binaryStorageExecutor shutdown, terminating. Had {} queued / {} active before interrupd; now have {} / {}", prevQueuedTaskCount, prevActiveTaskCount, binaryStorageExecutor.getQueue().size(), binaryStorageExecutor.getActiveCount());
terminateBinaryStorageExecutor();
}

LOGGER.info("Binary resource storage executor completed."); //$NON-NLS-1$

// Be ready to accept additional work
binaryStorageExecutor = makeBinaryStorageExecutor();
binaryStorageHighWaterMark = 0;
hwmTimeStamp = null;
}

@SuppressWarnings("nls")
private void terminateBinaryStorageExecutor() {
LOGGER.warn("Terminating binaryStorageExecutor");
List<Runnable> tasks = binaryStorageExecutor.shutdownNow();
LOGGER.warn("{} tasks not processed", tasks.size());
}

/**
* Updates the markers on a single resource.
*
Expand Down Expand Up @@ -873,8 +925,8 @@ protected IResourceDescription getSavedResourceDescription(final Map<URI, IResou
// TODO DSL-828: this may end up using a lot of memory; we should instead consider creating old copies of the resources in the db
IResourceDescription old = getResourceDescription(uri);
saved = old != null ? createOldStateResourceDescription(old) : null;
} else if (saved == NULL_DESCRIPTION) { // NOPMD
saved = null; // NOPMD
} else if (saved == NULL_DESCRIPTION) {
saved = null;
}
return saved;
}
Expand Down Expand Up @@ -986,9 +1038,10 @@ protected boolean recordDeltaAsNew(final Delta newDelta) {
* The progress monitor used for user feedback
* @return the list of {@link URI}s of loaded resources to be processed in the second phase
*/
private List<URI> writeResources(final Collection<URI> toWrite, final BuildData buildData, final IResourceDescriptions oldState, final CurrentDescriptions newState, final IProgressMonitor monitor) { // NOPMD
// NPath
// Complexity
@SuppressWarnings("PMD.AvoidInstanceofChecksInCatchClause")
private List<URI> writeResources(final Collection<URI> toWrite, final BuildData buildData, final IResourceDescriptions oldState, final CurrentDescriptions newState, final IProgressMonitor monitor) {
// NPath
// Complexity
ResourceSet resourceSet = buildData.getResourceSet();
IProject currentProject = getBuiltProject(buildData);
List<URI> toBuild = Lists.newLinkedList();
Expand Down Expand Up @@ -1029,7 +1082,7 @@ private List<URI> writeResources(final Collection<URI> toWrite, final BuildData
}
} catch (final WrappedException ex) {
pollForCancellation(monitor);
if (uri == null && ex instanceof LoadOperationException) { // NOPMD
if (uri == null && ex instanceof LoadOperationException) {
uri = ((LoadOperationException) ex).getUri();
}
LOGGER.error(NLS.bind(Messages.MonitoredClusteringBuilderState_CANNOT_LOAD_RESOURCE, uri != null ? uri : "unknown uri"), ex); //$NON-NLS-1$
Expand Down
7 changes: 5 additions & 2 deletions com.avaloq.tools.ddk.xtext/src/com/avaloq/tools/ddk/xtext/util/ParseTreeUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,11 @@ public static String getParsedString(final EObject object, final String featureN
public static void ensureNodeModelLoaded(final Resource resource) {
if (resource instanceof LazyLinkingResource2 lazyLinkinResource && lazyLinkinResource.isLoadedFromStorage()) {
EObject root = resource.getContents().get(0);
if (root != null && NodeModelUtils.getNode(root) instanceof ICompositeNode composite) {
Copy link
Copy Markdown
Member

@rubenporras rubenporras Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your version is fine for me, but what about just naming the variable rootNode instead of composite? That would be a smaller change than this one.

Copy link
Copy Markdown
Collaborator Author

@ava-fred ava-fred Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wouldn't fix the compiler error I see when I include the DDK in an ASMD workspace. The compiler complains because NodeModelUtils.getNode() returns an ICompositeNode, so the instanceof test is always true. The message is "Expression type cannot be a subtype of the Pattern type".

composite.getText(); // side-effect on removing com.avaloq.tools.ddk.xtext.resource.persistence.ProxyCompositeNode
if (root != null) {
ICompositeNode rootNode = NodeModelUtils.getNode(root);
if (rootNode != null) {
rootNode.getText(); // side-effect on removing com.avaloq.tools.ddk.xtext.resource.persistence.ProxyCompositeNode
}
}
}
}
Expand Down