Skip to content

Commit

Permalink
fix IProgressMonitor usage #335
Browse files Browse the repository at this point in the history 
passing a monitor to two callees requires it to be splitted.

#335

Also fixed forgotten monitor.done() in performProvisioningPlan and
improved tests.
  • Loading branch information
@EcljpseB0T
EcljpseB0T committed Sep 28, 2023
1 parent 0c02412 commit f724608
Show file tree
Hide file tree
Showing 9 changed files with 208 additions and 162 deletions.
94 changes: 52 additions & 42 deletions ....repository/src/org/eclipse/equinox/internal/p2/artifact/repository/RawMirrorRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,39 +46,43 @@ public RawMirrorRequest(IArtifactDescriptor sourceDescriptor, IArtifactDescripto

@Override
public void perform(IArtifactRepository sourceRepository, IProgressMonitor monitor) {
monitor.subTask(NLS.bind(Messages.downloading, getArtifactKey().getId()));
setSourceRepository(sourceRepository);
// Do we already have the descriptor in the target?
if (target.contains(targetDescriptor)) {
setResult(new Status(IStatus.INFO, Activator.ID, NLS.bind(Messages.mirror_alreadyExists, targetDescriptor, target)));
return;
}
// Does the source actually have the descriptor?
if (!source.contains(getArtifactDescriptor())) {
setResult(new Status(IStatus.ERROR, Activator.ID, NLS.bind(Messages.artifact_not_found, getArtifactKey())));
return;
}
IStatus status = transfer(targetDescriptor, sourceDescriptor, monitor);
SubMonitor subMon = SubMonitor.convert(monitor,NLS.bind(Messages.downloading, getArtifactKey().getId()),1);
try {
setSourceRepository(sourceRepository);
// Do we already have the descriptor in the target?
if (target.contains(targetDescriptor)) {
setResult(new Status(IStatus.INFO, Activator.ID, NLS.bind(Messages.mirror_alreadyExists, targetDescriptor, target)));
return;
}
// Does the source actually have the descriptor?
if (!source.contains(getArtifactDescriptor())) {
setResult(new Status(IStatus.ERROR, Activator.ID, NLS.bind(Messages.artifact_not_found, getArtifactKey())));
return;
}
IStatus status = transfer(targetDescriptor, sourceDescriptor, subMon.split(1));

// if ok, cancelled or transfer has already been done with the canonical form return with status set
if (status.getSeverity() == IStatus.CANCEL) {
setResult(status);
return;
}
if (monitor.isCanceled()) {
setResult(Status.CANCEL_STATUS);
return;
}
if (status.isOK()) {
setResult(status);
return;
}
// if ok, cancelled or transfer has already been done with the canonical form return with status set
if (status.getSeverity() == IStatus.CANCEL) {
setResult(status);
return;
}
if (monitor.isCanceled()) {
setResult(Status.CANCEL_STATUS);
return;
}
if (status.isOK()) {
setResult(status);
return;
}

// failed, first remove possibly erroneously added descriptor
if (target.contains(targetDescriptor))
target.removeDescriptor(targetDescriptor);
// failed, first remove possibly erroneously added descriptor
if (target.contains(targetDescriptor))
target.removeDescriptor(targetDescriptor);

setResult(status);
setResult(status);
} finally {
subMon.done();
}
}

public IArtifactDescriptor getArtifactDescriptor() {
Expand All @@ -88,19 +92,25 @@ public IArtifactDescriptor getArtifactDescriptor() {
// Perform the mirror operation without any processing steps
@Override
protected IStatus getArtifact(IArtifactDescriptor artifactDescriptor, OutputStream destination, IProgressMonitor monitor) {
if (SimpleArtifactRepository.CHECKSUMS_ENABLED) {
Collection<ChecksumVerifier> steps = ChecksumUtilities.getChecksumVerifiers(artifactDescriptor,
IArtifactDescriptor.DOWNLOAD_CHECKSUM, Collections.emptySet());
if (steps.isEmpty()) {
LogHelper.log(new Status(IStatus.WARNING, Activator.ID,
NLS.bind(Messages.noDigestAlgorithmToVerifyDownload, artifactDescriptor.getArtifactKey())));
monitor = IProgressMonitor.nullSafe(monitor);
try {
SubMonitor subMon = SubMonitor.convert(monitor, 2);
if (SimpleArtifactRepository.CHECKSUMS_ENABLED) {
Collection<ChecksumVerifier> steps = ChecksumUtilities.getChecksumVerifiers(artifactDescriptor,
IArtifactDescriptor.DOWNLOAD_CHECKSUM, Collections.emptySet());
if (steps.isEmpty()) {
LogHelper.log(new Status(IStatus.WARNING, Activator.ID,
NLS.bind(Messages.noDigestAlgorithmToVerifyDownload, artifactDescriptor.getArtifactKey())));
}
ProcessingStep[] stepArray = steps.toArray(new ProcessingStep[steps.size()]);
// TODO should probably be using createAndLink here
ProcessingStepHandler handler = new ProcessingStepHandler();
destination = handler.link(stepArray, destination, subMon.split(1));
}
ProcessingStep[] stepArray = steps.toArray(new ProcessingStep[steps.size()]);
// TODO should probably be using createAndLink here
ProcessingStepHandler handler = new ProcessingStepHandler();
destination = handler.link(stepArray, destination, monitor);
subMon.setWorkRemaining(1);
return getSourceRepository().getRawArtifact(artifactDescriptor, destination, subMon.split(1));
} finally {
monitor.done();
}

return getSourceRepository().getRawArtifact(artifactDescriptor, destination, monitor);
}
}
103 changes: 54 additions & 49 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 @@ -615,58 +615,63 @@ private boolean doRemoveArtifact(IArtifactDescriptor descriptor) {

protected IStatus downloadArtifact(IArtifactDescriptor descriptor, OutputStream destination, IProgressMonitor monitor) {
monitor = IProgressMonitor.nullSafe(monitor);
if (isFolderBased(descriptor)) {
File artifactFolder = getArtifactFile(descriptor);
if (artifactFolder == null) {
if (getLocation(descriptor) != null && !URIUtil.isFileURI(getLocation(descriptor)))
return reportStatus(descriptor, destination, new Status(IStatus.ERROR, Activator.ID, NLS.bind(Messages.folder_artifact_not_file_repo, descriptor.getArtifactKey())));
return reportStatus(descriptor, destination, new Status(IStatus.ERROR, Activator.ID, NLS.bind(Messages.artifact_not_found, descriptor.getArtifactKey())));
}
// TODO: optimize and ensure manifest is written first
File zipFile = null;
long start = System.currentTimeMillis();
long totalArtifactSize = 0;
try {
zipFile = File.createTempFile(artifactFolder.getName(), JAR_EXTENSION, null);
FileUtils.zip(artifactFolder.listFiles(), null, zipFile, FileUtils.createRootPathComputer(artifactFolder));
FileInputStream fis = new FileInputStream(zipFile);
totalArtifactSize += FileUtils.copyStream(fis, true, destination, false);
} catch (IOException e) {
return reportStatus(descriptor, destination, new Status(IStatus.ERROR, Activator.ID, e.getMessage(), e));
} finally {
if (zipFile != null)
zipFile.delete();
}
long end = System.currentTimeMillis();
DownloadStatus statusWithDownloadSpeed = new DownloadStatus(IStatus.OK, Activator.ID, Status.OK_STATUS.getMessage());
try {
statusWithDownloadSpeed.setFileSize(totalArtifactSize);
statusWithDownloadSpeed.setTransferRate(totalArtifactSize / Math.max((end - start), 1) * 1000);
} catch (NumberFormatException e) {
// ignore
try {
SubMonitor subMon = SubMonitor.convert(monitor, 2);
if (isFolderBased(descriptor)) {
File artifactFolder = getArtifactFile(descriptor);
if (artifactFolder == null) {
if (getLocation(descriptor) != null && !URIUtil.isFileURI(getLocation(descriptor)))
return reportStatus(descriptor, destination, new Status(IStatus.ERROR, Activator.ID, NLS.bind(Messages.folder_artifact_not_file_repo, descriptor.getArtifactKey())));
return reportStatus(descriptor, destination, new Status(IStatus.ERROR, Activator.ID, NLS.bind(Messages.artifact_not_found, descriptor.getArtifactKey())));
}
// TODO: optimize and ensure manifest is written first
File zipFile = null;
long start = System.currentTimeMillis();
long totalArtifactSize = 0;
try {
zipFile = File.createTempFile(artifactFolder.getName(), JAR_EXTENSION, null);
FileUtils.zip(artifactFolder.listFiles(), null, zipFile, FileUtils.createRootPathComputer(artifactFolder));
FileInputStream fis = new FileInputStream(zipFile);
totalArtifactSize += FileUtils.copyStream(fis, true, destination, false);
} catch (IOException e) {
return reportStatus(descriptor, destination, new Status(IStatus.ERROR, Activator.ID, e.getMessage(), e));
} finally {
if (zipFile != null)
zipFile.delete();
}
long end = System.currentTimeMillis();
DownloadStatus statusWithDownloadSpeed = new DownloadStatus(IStatus.OK, Activator.ID, Status.OK_STATUS.getMessage());
try {
statusWithDownloadSpeed.setFileSize(totalArtifactSize);
statusWithDownloadSpeed.setTransferRate(totalArtifactSize / Math.max((end - start), 1) * 1000);
} catch (NumberFormatException e) {
// ignore
}
return reportStatus(descriptor, destination, statusWithDownloadSpeed);
}
return reportStatus(descriptor, destination, statusWithDownloadSpeed);
}

//download from the best available mirror
URI baseLocation = getLocation(descriptor);
if (baseLocation == null)
return new Status(IStatus.ERROR, Activator.ID, NLS.bind(Messages.no_location, descriptor));
URI mirrorLocation = getMirror(baseLocation, monitor);
IStatus status = downloadArtifact(mirrorLocation, destination, monitor);
IStatus result = reportStatus(descriptor, destination, status);
// if the original download went reasonably but the reportStatus found some issues
// (e..g, in the processing steps/validators) then mark the mirror as bad and return
// a retry code (assuming we have more mirrors)
if ((status.isOK() || status.matches(IStatus.INFO | IStatus.WARNING)) && result.getSeverity() == IStatus.ERROR && !artifactError(result)) {
if (mirrors != null) {
mirrors.reportResult(mirrorLocation.toString(), result);
if (mirrors.hasValidMirror())
return new MultiStatus(Activator.ID, CODE_RETRY, new IStatus[] {result}, "Retry another mirror", null); //$NON-NLS-1$

//download from the best available mirror
URI baseLocation = getLocation(descriptor);
if (baseLocation == null)
return new Status(IStatus.ERROR, Activator.ID, NLS.bind(Messages.no_location, descriptor));
URI mirrorLocation = getMirror(baseLocation, subMon.split(1));
IStatus status = downloadArtifact(mirrorLocation, destination, subMon.split(1));
IStatus result = reportStatus(descriptor, destination, status);
// if the original download went reasonably but the reportStatus found some issues
// (e..g, in the processing steps/validators) then mark the mirror as bad and return
// a retry code (assuming we have more mirrors)
if ((status.isOK() || status.matches(IStatus.INFO | IStatus.WARNING)) && result.getSeverity() == IStatus.ERROR && !artifactError(result)) {
if (mirrors != null) {
mirrors.reportResult(mirrorLocation.toString(), result);
if (mirrors.hasValidMirror())
return new MultiStatus(Activator.ID, CODE_RETRY, new IStatus[] {result}, "Retry another mirror", null); //$NON-NLS-1$
}
}
// if the original status was a retry, don't lose that.
return status.getCode() == CODE_RETRY ? status : result;
} finally {
monitor.done();
}
// if the original status was a retry, don't lose that.
return status.getCode() == CODE_RETRY ? status : result;
}

/**
Expand Down
12 changes: 7 additions & 5 deletions bundles/org.eclipse.equinox.p2.engine/src/org/eclipse/equinox/internal/p2/engine/Engine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ public IStatus perform(IProfile iprofile, IPhaseSet phases, Operand[] operands,
Profile profile = profileRegistry.validate(iprofile);

profileRegistry.lockProfile(profile);
SubMonitor subMon = SubMonitor.convert(monitor, 3);
try {
eventBus.publishEvent(new BeginOperationEvent(profile, phaseSet, operands, this));
if (DebugHelper.DEBUG_ENGINE)
Expand All @@ -91,17 +92,17 @@ public IStatus perform(IProfile iprofile, IPhaseSet phases, Operand[] operands,
}
context.setProperty(ProvisioningContext.CHECK_AUTHORITIES, property);
}

MultiStatus result = phaseSet.perform(session, operands, monitor);
MultiStatus result = phaseSet.perform(session, operands, subMon.split(1));
if (result.isOK() || result.matches(IStatus.INFO | IStatus.WARNING)) {
if (DebugHelper.DEBUG_ENGINE)
DebugHelper.debug(ENGINE, "Preparing to commit engine operation for profile=" + profile.getProfileId()); //$NON-NLS-1$
result.merge(session.prepare(monitor));
result.merge(session.prepare(subMon.split(1)));
}
subMon.setWorkRemaining(1);
if (result.matches(IStatus.ERROR | IStatus.CANCEL)) {
if (DebugHelper.DEBUG_ENGINE)
DebugHelper.debug(ENGINE, "Rolling back engine operation for profile=" + profile.getProfileId() + ". Reason was: " + result.toString()); //$NON-NLS-1$ //$NON-NLS-2$
IStatus status = session.rollback(monitor, result.getSeverity());
IStatus status = session.rollback(subMon.split(1), result.getSeverity());
if (status.matches(IStatus.ERROR))
LogHelper.log(status);
eventBus.publishEvent(new RollbackOperationEvent(profile, phaseSet, operands, this, result));
Expand All @@ -110,7 +111,7 @@ public IStatus perform(IProfile iprofile, IPhaseSet phases, Operand[] operands,
DebugHelper.debug(ENGINE, "Committing engine operation for profile=" + profile.getProfileId()); //$NON-NLS-1$
if (profile.isChanged())
profileRegistry.updateProfile(profile);
IStatus status = session.commit(monitor);
IStatus status = session.commit(subMon.split(1));
if (status.matches(IStatus.ERROR))
LogHelper.log(status);
eventBus.publishEvent(new CommitOperationEvent(profile, phaseSet, operands, this));
Expand All @@ -121,6 +122,7 @@ public IStatus perform(IProfile iprofile, IPhaseSet phases, Operand[] operands,
} finally {
profileRegistry.unlockProfile(profile);
profile.setChanged(false);
monitor.done();
}
}

Expand Down
28 changes: 17 additions & 11 deletions ...e.equinox.p2.operations/src/org/eclipse/equinox/p2/operations/ProfileChangeOperation.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,18 +110,24 @@ protected ProfileChangeOperation(ProvisioningSession session) {
* @return a status describing the resolution results
*/
public final IStatus resolveModal(IProgressMonitor monitor) {
if (monitor == null)
monitor = new NullProgressMonitor();
prepareToResolve();
makeResolveJob(monitor);
if (job != null) {
IStatus status = job.runModal(monitor);
if (status.getSeverity() == IStatus.CANCEL)
return Status.CANCEL_STATUS;
try {
if (monitor == null)
monitor = new NullProgressMonitor();
prepareToResolve();
SubMonitor subMon = SubMonitor.convert(monitor, 2);
makeResolveJob(subMon.split(1));
if (job != null) {
IStatus status = job.runModal(subMon.split(1));
if (status.getSeverity() == IStatus.CANCEL)
return Status.CANCEL_STATUS;
}
// For anything other than cancellation, we examine the artifacts of the
// resolution and come
// up with an overall summary.
return getResolutionResult();
} finally {
monitor.done();
}
// For anything other than cancellation, we examine the artifacts of the resolution and come
// up with an overall summary.
return getResolutionResult();

}

Expand Down
4 changes: 2 additions & 2 deletions ...e.equinox.p2.operations/src/org/eclipse/equinox/p2/operations/ProfileModificationJob.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,9 @@ public IStatus runModal(IProgressMonitor monitor) {
IStatus status = Status.OK_STATUS;
if (task == null)
task = getName();
monitor.beginTask(task, 1000);
try {
status = getSession().performProvisioningPlan(plan, phaseSet, provisioningContext, SubMonitor.convert(monitor, 1000));
status = getSession().performProvisioningPlan(plan, phaseSet, provisioningContext,
SubMonitor.convert(monitor, task, 1000));
} finally {
monitor.done();
}
Expand Down
Loading

0 comments on commit f724608

Please sign in to comment.