From c9c4ea293ed3a5aae88c349c28915075d071cbd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20L=C3=A4ubrich?= Date: Fri, 20 Dec 2024 08:30:34 +0100 Subject: [PATCH] Improve progress reporting for download jobs Currently the reporting of progress for download jobs has some flaws: 1) it concurrently updates a shared monitor but these are usually not made for concurrent use 2) the jobs own monitor does not really reflect progress 3) Messages are not externalized This now refactor this part in the following way: - create a submonitor from the job so we can update the remaning work - assign some subticks to each download to report progress on the job - set the current artifact behind downloaded as the message - report messages from downstream as sub task to the job - add two consumers for the caller of the job to get notified about messages and status --- .../p2/artifact/repository/Messages.java | 3 + .../artifact/repository/messages.properties | 4 +- .../repository/simple/DownloadJob.java | 116 ++++++++++++++---- .../simple/SimpleArtifactRepository.java | 48 ++++++-- 4 files changed, 138 insertions(+), 33 deletions(-) diff --git a/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/Messages.java b/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/Messages.java index 30d4d941a4..9469e28849 100644 --- a/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/Messages.java +++ b/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/Messages.java @@ -18,6 +18,9 @@ public class Messages extends NLS { private static final String BUNDLE_NAME = "org.eclipse.equinox.internal.p2.artifact.repository.messages"; //$NON-NLS-1$ + public static String DownloadJob_initial; + public static String DownloadJob_current_artifact; + public static String artifact_not_found; public static String available_already_in; public static String no_location; diff --git a/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/messages.properties b/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/messages.properties index e83d232ef9..a5643b5ed8 100644 --- a/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/messages.properties +++ b/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/messages.properties @@ -66,4 +66,6 @@ retryRequest=Download of {0} failed on repository {1}. Retrying. error_copying_local_file=An error occurred copying file {0}. onlyInsecureDigestAlgorithmUsed = The digest algorithms ({0}) used to verify {1} have severely compromised security. Please report this concern to the artifact provider. -noDigestAlgorithmToVerifyDownload = No digest algorithm is available to verify download of {0} from repository {1}. \ No newline at end of file +noDigestAlgorithmToVerifyDownload = No digest algorithm is available to verify download of {0} from repository {1}. +DownloadJob_initial=Downloading Software +DownloadJob_current_artifact=Downloading {0} \ No newline at end of file diff --git a/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/simple/DownloadJob.java b/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/simple/DownloadJob.java index f4c5b89c99..d2a53a2502 100644 --- a/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/simple/DownloadJob.java +++ b/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/simple/DownloadJob.java @@ -15,26 +15,34 @@ package org.eclipse.equinox.internal.p2.artifact.repository.simple; import java.util.LinkedList; +import java.util.function.Consumer; import org.eclipse.core.runtime.*; import org.eclipse.core.runtime.jobs.Job; +import org.eclipse.equinox.internal.p2.artifact.repository.Messages; +import org.eclipse.equinox.p2.metadata.IArtifactKey; import org.eclipse.equinox.p2.repository.artifact.IArtifactRequest; +import org.eclipse.osgi.util.NLS; public class DownloadJob extends Job { + private static final int SUB_TICKS = 1000; + static final Object FAMILY = new Object(); private final LinkedList requestsPending; private final SimpleArtifactRepository repository; - private final IProgressMonitor masterMonitor; - private final MultiStatus overallStatus; + + private Consumer resultConsumer; + + private Consumer messageConsumer; DownloadJob(String name, SimpleArtifactRepository repository, LinkedList requestsPending, - IProgressMonitor masterMonitor, MultiStatus overallStatus) { + Consumer resultConsumer, Consumer messageConsumer) { super(name); + this.resultConsumer = resultConsumer; + this.messageConsumer = messageConsumer; setSystem(true); this.repository = repository; this.requestsPending = requestsPending; - this.masterMonitor = masterMonitor; - this.overallStatus = overallStatus; } @Override @@ -44,28 +52,94 @@ public boolean belongsTo(Object family) { @Override protected IStatus run(IProgressMonitor jobMonitor) { - jobMonitor.beginTask("Downloading software", IProgressMonitor.UNKNOWN); //$NON-NLS-1$ + SubMonitor monitor = SubMonitor.convert(jobMonitor, Messages.DownloadJob_initial, 1_000_000); do { - // get the request we are going to process IArtifactRequest request; synchronized (requestsPending) { - if (requestsPending.isEmpty()) - break; + int totalDownloadWork = requestsPending.size(); + if (totalDownloadWork == 0) { + return Status.OK_STATUS; + } + monitor.setWorkRemaining(totalDownloadWork * SUB_TICKS); request = requestsPending.removeFirst(); } - if (masterMonitor.isCanceled()) - return Status.CANCEL_STATUS; - // process the actual request - SubMonitor subMonitor = SubMonitor.convert(masterMonitor.slice(1), 1); - IStatus status = repository.getArtifact(request, subMonitor); - if (!status.isOK()) { - synchronized (overallStatus) { - overallStatus.add(status); + IArtifactKey key = request.getArtifactKey(); + String currentArtifact = String.format("%s %s", key.getId(), key.getVersion()); //$NON-NLS-1$ + messageConsumer.accept(NLS.bind(Messages.DownloadJob_current_artifact, currentArtifact)); + monitor.setTaskName(currentArtifact); + SubMonitor split = monitor.split(SUB_TICKS, SubMonitor.SUPPRESS_NONE); + IStatus status = repository.getArtifact(request, new IProgressMonitor() { + + private volatile boolean canceled; + private String taskName; + private String subTaskName; + + @Override + public void worked(int work) { + split.worked(work); } - } - } while (true); - jobMonitor.done(); - return Status.OK_STATUS; + @Override + public void subTask(String name) { + this.subTaskName = name; + updateTaskName(); + } + + @Override + public void setTaskName(String name) { + this.taskName = name; + updateTaskName(); + } + + @Override + public void setCanceled(boolean canceled) { + this.canceled = canceled; + } + + @Override + public boolean isCanceled() { + return canceled; + } + + @Override + public void internalWorked(double work) { + split.internalWorked(work); + } + + @Override + public void done() { + split.done(); + + } + + @Override + public void beginTask(String name, int totalWork) { + monitor.beginTask(name, totalWork); + this.taskName = name; + updateTaskName(); + } + + private void updateTaskName() { + StringBuilder sb = new StringBuilder(); + if (taskName != null && !taskName.isBlank()) { + sb.append(taskName); + } + if (subTaskName != null && !subTaskName.isBlank()) { + if (sb.length() > 0) { + sb.append(" - "); //$NON-NLS-1$ + } + sb.append(subTaskName); + } + String message = sb.toString(); + if (message.length() > 0) { + messageConsumer.accept(message); + monitor.subTask(message); + } + } + }); + resultConsumer.accept(status); + } while (!monitor.isCanceled()); + return Status.CANCEL_STATUS; } + } diff --git a/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/simple/SimpleArtifactRepository.java b/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/simple/SimpleArtifactRepository.java index edb44e52b8..dece436c13 100644 --- a/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/simple/SimpleArtifactRepository.java +++ b/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/simple/SimpleArtifactRepository.java @@ -24,6 +24,8 @@ import java.net.URISyntaxException; import java.util.*; import java.util.Map.Entry; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.function.Consumer; import java.util.jar.JarEntry; import java.util.jar.JarOutputStream; import org.eclipse.core.runtime.*; @@ -887,15 +889,16 @@ public File getArtifactFile(IArtifactKey key) { } @Override - public IStatus getArtifacts(IArtifactRequest[] requests, IProgressMonitor monitor) { - monitor = IProgressMonitor.nullSafe(monitor); + public IStatus getArtifacts(IArtifactRequest[] requests, IProgressMonitor unsafeMonitor) { + IProgressMonitor monitor = IProgressMonitor.nullSafe(unsafeMonitor); if (!holdsLock() && URIUtil.isFileURI(getLocation())) { load(new NullProgressMonitor()); } if (monitor.isCanceled()) return Status.CANCEL_STATUS; - final MultiStatus overallStatus = new MultiStatus(Activator.ID, IStatus.OK, NLS.bind(Messages.message_problemReadingArtifact, getLocation()), null); + final MultiStatus overallStatus = new MultiStatus(Activator.ID, IStatus.OK, + NLS.bind(Messages.message_problemReadingArtifact, getLocation()), null); LinkedList requestsPending = new LinkedList<>(Arrays.asList(requests)); int numberOfJobs = Math.min(requests.length, getMaximumThreads()); @@ -903,11 +906,13 @@ public IStatus getArtifacts(IArtifactRequest[] requests, IProgressMonitor monito SubMonitor subMonitor = SubMonitor.convert(monitor, requests.length); try { for (IArtifactRequest request : requests) { - if (monitor.isCanceled()) + if (monitor.isCanceled()) { return Status.CANCEL_STATUS; + } IStatus result = getArtifact(request, subMonitor.newChild(1)); - if (!result.isOK()) + if (!result.isOK()) { overallStatus.add(result); + } } } finally { subMonitor.done(); @@ -917,28 +922,49 @@ public IStatus getArtifacts(IArtifactRequest[] requests, IProgressMonitor monito monitor.beginTask(NLS.bind(Messages.sar_downloading, Integer.toString(requests.length)), requests.length); try { DownloadJob jobs[] = new DownloadJob[numberOfJobs]; + List jobStatus = new CopyOnWriteArrayList<>(); + Consumer resultConsumer = result -> { + synchronized (monitor) { + monitor.worked(1); + } + if (!result.isOK()) { + jobStatus.add(result); + } + }; + Consumer messageConsumer = jobMsg -> { + synchronized (monitor) { + // last message wins + monitor.subTask(jobMsg); + } + }; for (int i = 0; i < numberOfJobs; i++) { - jobs[i] = new DownloadJob(Messages.sar_downloadJobName + i, this, requestsPending, monitor, - overallStatus); + jobs[i] = new DownloadJob(Messages.sar_downloadJobName + i, this, requestsPending, resultConsumer, + messageConsumer); jobs[i].schedule(); } // wait for all the jobs to complete try { Job.getJobManager().join(DownloadJob.FAMILY, null); } catch (InterruptedException e) { - //ignore + // ignore } + overallStatus.addAll(overallStatus); } finally { monitor.done(); } } - if (monitor.isCanceled()) + if (monitor.isCanceled()) { return Status.CANCEL_STATUS; - else if (overallStatus.isOK()) + } else if (overallStatus.isOK()) { return Status.OK_STATUS; - else + } else { + IStatus[] children = overallStatus.getChildren(); + if (children.length == 1) { + return children[0]; + } return overallStatus; + } } public synchronized IArtifactDescriptor getCompleteArtifactDescriptor(IArtifactKey key) {