Skip to content

Commit

Permalink
Improve progress reporting for download jobs
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@laeubi @merks
laeubi authored and merks committed Dec 27, 2024
1 parent 93e609c commit 70dc6a5
Show file tree
Hide file tree
Showing 5 changed files with 146 additions and 36 deletions.
3 changes: 3 additions & 0 deletions ...artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/Messages.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 3 additions & 1 deletion ...ct.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/messages.properties
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}.
noDigestAlgorithmToVerifyDownload = No digest algorithm is available to verify download of {0} from repository {1}.
DownloadJob_initial=Downloading Software
DownloadJob_current_artifact=Downloading {0}
118 changes: 97 additions & 21 deletions ...epository/src/org/eclipse/equinox/internal/p2/artifact/repository/simple/DownloadJob.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,26 +15,33 @@
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;

public class DownloadJob extends Job {
private static final int SUB_TICKS = 1000;

static final Object FAMILY = new Object();

private final LinkedList<IArtifactRequest> requestsPending;
private final SimpleArtifactRepository repository;
private final IProgressMonitor masterMonitor;
private final MultiStatus overallStatus;

private Consumer<IStatus> resultConsumer;

private Consumer<String> messageConsumer;

DownloadJob(String name, SimpleArtifactRepository repository, LinkedList<IArtifactRequest> requestsPending,
IProgressMonitor masterMonitor, MultiStatus overallStatus) {
Consumer<IStatus> resultConsumer, Consumer<String> messageConsumer) {
super(name);
this.resultConsumer = resultConsumer;
this.messageConsumer = messageConsumer;
setSystem(true);
this.repository = repository;
this.requestsPending = requestsPending;
this.masterMonitor = masterMonitor;
this.overallStatus = overallStatus;
}

@Override
Expand All @@ -44,28 +51,97 @@ 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$
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;
private String lastMessage;

@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;
if (messageConsumer != null) {
messageConsumer.accept(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 && !message.equals(lastMessage)) {
lastMessage = message;
monitor.subTask(message);
}
}
});
resultConsumer.accept(status);
} while (!monitor.isCanceled());
return Status.CANCEL_STATUS;
}

}
48 changes: 37 additions & 11 deletions .../org/eclipse/equinox/internal/p2/artifact/repository/simple/SimpleArtifactRepository.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.*;
Expand Down Expand Up @@ -887,27 +889,30 @@ 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<IArtifactRequest> requestsPending = new LinkedList<>(Arrays.asList(requests));

int numberOfJobs = Math.min(requests.length, getMaximumThreads());
if (numberOfJobs <= 1 || (!isForceThreading() && isLocal())) {
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();
Expand All @@ -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<IStatus> jobStatus = new CopyOnWriteArrayList<>();
Consumer<IStatus> resultConsumer = result -> {
synchronized (monitor) {
monitor.worked(1);
}
if (!result.isOK()) {
jobStatus.add(result);
}
};
Consumer<String> 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) {
Expand Down
9 changes: 6 additions & 3 deletions ...src/org/eclipse/equinox/p2/tests/artifact/repository/CompositeArtifactRepositoryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,8 @@ public void testGetArtifactsFromRequests() {
}

public void testGetArtifactsWithErrorInChild() throws Exception {
repositoryURI = getTestData("1", "/testData/artifactRepo/composite/errorInChild").toURI();
File testData = getTestData("1", "/testData/artifactRepo/composite/errorInChild");
repositoryURI = testData.toURI();
IArtifactRepository repo = getArtifactRepositoryManager().loadRepository(repositoryURI, null);

IArtifactRequest[] requests = new IArtifactRequest[] {new ArtifactRequest(new ArtifactKey("osgi.bundle", "plugin", Version.parseVersion("1.0.0")), null) {
Expand All @@ -611,8 +612,10 @@ public void perform(IArtifactRepository sourceRepository, IProgressMonitor monit
assertThat(status, is(statusWithMessageWhich(containsString("while reading artifacts from child repositories"))));

// bug 391400: status should point to repository with problem
String brokenChildURI = repositoryURI.toString() + "child";
assertThat(Arrays.asList(status.getChildren()), hasItem(statusWithMessageWhich(containsString(brokenChildURI))));
assertThat(Arrays.asList(status.getChildren()),
hasItem(statusWithMessageWhich(containsString("An error occurred copying file"))));
assertThat(Arrays.asList(status.getChildren()),
hasItem(statusWithMessageWhich(containsString(testData.getAbsolutePath()))));
}

public void testLoadingRepositoryRemote() {
Expand Down

0 comments on commit 70dc6a5

Please sign in to comment.