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 @HannesWell
EcljpseB0T authored and HannesWell committed Nov 5, 2023
1 parent 2ed3a71 commit e31c371
Show file tree
Hide file tree
Showing 12 changed files with 97 additions and 89 deletions.
13 changes: 7 additions & 6 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,19 +46,19 @@ public RawMirrorRequest(IArtifactDescriptor sourceDescriptor, IArtifactDescripto

@Override
public void perform(IArtifactRepository sourceRepository, IProgressMonitor monitor) {
monitor.subTask(NLS.bind(Messages.downloading, getArtifactKey().getId()));
SubMonitor subMon = SubMonitor.convert(monitor, NLS.bind(Messages.downloading, getArtifactKey().getId()), 1);
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)));
setResult(Status.info(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);
IStatus status = transfer(targetDescriptor, sourceDescriptor, subMon.newChild(1));

// if ok, cancelled or transfer has already been done with the canonical form return with status set
if (status.getSeverity() == IStatus.CANCEL) {
Expand Down Expand Up @@ -88,6 +88,7 @@ public IArtifactDescriptor getArtifactDescriptor() {
// Perform the mirror operation without any processing steps
@Override
protected IStatus getArtifact(IArtifactDescriptor artifactDescriptor, OutputStream destination, IProgressMonitor monitor) {
SubMonitor subMon = SubMonitor.convert(monitor, 2);
if (SimpleArtifactRepository.CHECKSUMS_ENABLED) {
Collection<ChecksumVerifier> steps = ChecksumUtilities.getChecksumVerifiers(artifactDescriptor,
IArtifactDescriptor.DOWNLOAD_CHECKSUM, Collections.emptySet());
Expand All @@ -98,9 +99,9 @@ protected IStatus getArtifact(IArtifactDescriptor artifactDescriptor, OutputStre
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);
destination = handler.link(stepArray, destination, subMon.split(1));
}

return getSourceRepository().getRawArtifact(artifactDescriptor, destination, monitor);
subMon.setWorkRemaining(1);
return getSourceRepository().getRawArtifact(artifactDescriptor, destination, subMon.split(1));
}
}
6 changes: 3 additions & 3 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 @@ -617,7 +617,7 @@ private boolean doRemoveArtifact(IArtifactDescriptor descriptor) {
}

protected IStatus downloadArtifact(IArtifactDescriptor descriptor, OutputStream destination, IProgressMonitor monitor) {
monitor = IProgressMonitor.nullSafe(monitor);
SubMonitor subMon = SubMonitor.convert(monitor, 2);
if (isFolderBased(descriptor)) {
File artifactFolder = getArtifactFile(descriptor);
if (artifactFolder == null) {
Expand Down Expand Up @@ -655,8 +655,8 @@ protected IStatus downloadArtifact(IArtifactDescriptor descriptor, OutputStream
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);
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
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
90 changes: 45 additions & 45 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 @@ -28,7 +28,7 @@
* resolution status to be reported to a client to determine whether the operation
* should proceed. Each phase of the operation can be performed synchronously or in
* the background as a job. To perform the operation synchronously:
*
*
* <pre>
* IStatus result = op.resolveModal(monitor);
* if (result.isOK())
Expand All @@ -37,10 +37,10 @@
* // interpret the result
* }
* </pre>
*
*
* To perform the resolution synchronously and the provisioning job in the
* background:
*
*
* <pre>
* IStatus status = op.resolveModal(monitor);
* if (status.isOK()) {
Expand All @@ -50,9 +50,9 @@
* // interpret the result
* }
* </pre>
*
*
* To resolve in the background and perform the job when it is complete:
*
*
* <pre>
* ProvisioningJob job = op.getResolveJob(monitor);
* job.addJobChangeListener(new JobChangeAdapter() {
Expand All @@ -65,18 +65,18 @@
* }
* });
* job.schedule();
*
*
* </pre>
*
* In general, it is expected that clients create a new ProfileChangeOperation if
*
* In general, it is expected that clients create a new ProfileChangeOperation if
* the resolution result of the current operation is not satisfactory. However,
* subclasses may prescribe a different life cycle where appropriate.
*
* When retrieving the resolution and provisioning jobs managed by this operation,
*
* When retrieving the resolution and provisioning jobs managed by this operation,
* a client may supply a progress monitor to be used by the job. When the job is
* run by the platform job manager, both the platform job manager progress indicator
* run by the platform job manager, both the platform job manager progress indicator
* and the monitor supplied by the client will be updated.
*
*
* @noextend This class is not intended to be subclassed by clients.
* @since 2.0
*/
Expand All @@ -93,7 +93,7 @@ public abstract class ProfileChangeOperation implements IProfileChangeJob {
* Create an operation using the provided provisioning session.
* Unless otherwise specified by the client, the operation is
* performed on the currently running profile.
*
*
* @param session the provisioning session providing the services
*/
protected ProfileChangeOperation(ProvisioningSession session) {
Expand All @@ -105,20 +105,20 @@ protected ProfileChangeOperation(ProvisioningSession session) {
/**
* Resolve the operation in the current thread using the specified progress
* monitor. Return a status describing the result of the resolution.
*
*
* @param monitor the progress monitor to use
* @return a status describing the resolution results
*/
public final IStatus resolveModal(IProgressMonitor monitor) {
if (monitor == null)
monitor = new NullProgressMonitor();
SubMonitor subMon = SubMonitor.convert(monitor, 2);
prepareToResolve();
makeResolveJob(monitor);
makeResolveJob(subMon.split(1));
if (job != null) {
IStatus status = job.runModal(monitor);
IStatus status = job.runModal(subMon.split(1));
if (status.getSeverity() == IStatus.CANCEL)
return Status.CANCEL_STATUS;
}
monitor.done();
// For anything other than cancellation, we examine the artifacts of the resolution and come
// up with an overall summary.
return getResolutionResult();
Expand All @@ -135,11 +135,11 @@ public void setProfileId(String id) {

/**
* Return a job that can be used to resolve this operation in the background.
*
*
* @param monitor a progress monitor that should be used to report the job's progress in addition
* to the standard job progress reporting. Can be <code>null</code>. If provided, this monitor
* to the standard job progress reporting. Can be <code>null</code>. If provided, this monitor
* will be called from a background thread.
*
*
* @return a job that can be scheduled to perform the provisioning operation.
*/
public final ProvisioningJob getResolveJob(IProgressMonitor monitor) {
Expand Down Expand Up @@ -178,11 +178,11 @@ void makeResolveJob(IProgressMonitor monitor) {

/**
* Compute the profile change request for this operation, adding any relevant intermediate status
* to the supplied status.
*
* to the supplied status.
*
* @param status a multi-status to be used to add relevant status. If a profile change request cannot
* be computed for any reason, a status should be added to explain the problem.
*
*
* @param monitor the progress monitor to use for computing the profile change request
*/
protected abstract void computeProfileChangeRequest(MultiStatus status, IProgressMonitor monitor);
Expand All @@ -193,14 +193,14 @@ private void createPlannerResolutionJob() {

/**
* Return an appropriate name for the resolution job.
*
*
* @return the resolution job name.
*/
protected abstract String getResolveJobName();

/**
* Return an appropriate name for the provisioning job.
*
*
* @return the provisioning job name.
*/
protected abstract String getProvisioningJobName();
Expand All @@ -209,7 +209,7 @@ private void createPlannerResolutionJob() {
* Return a status indicating the result of resolving this
* operation. A <code>null</code> return indicates that
* resolving has not occurred yet.
*
*
* @return the status of the resolution, or <code>null</code>
* if resolution has not yet occurred.
*/
Expand All @@ -231,7 +231,7 @@ public IStatus getResolutionResult() {
/**
* Return a string that can be used to describe the results of the resolution
* to a client.
*
*
* @return a string describing the resolution details, or <code>null</code> if the
* operation has not been resolved.
*/
Expand All @@ -250,9 +250,9 @@ public String getResolutionDetails() {
/**
* Return a string that describes the specific resolution results
* related to the supplied {@link IInstallableUnit}.
*
*
* @param iu the IInstallableUnit for which resolution details are requested
*
*
* @return a string describing the results for the installable unit, or <code>null</code> if
* there are no specific results available for the installable unit.
*/
Expand All @@ -265,12 +265,12 @@ public String getResolutionDetails(IInstallableUnit iu) {

/**
* Return the provisioning plan obtained by resolving the receiver.
*
*
* @return the provisioning plan. This may be <code>null</code> if the operation
* has not been resolved, or if a plan could not be obtained when attempting to
* resolve. If the plan is null and the operation has been resolved, then the
* resolution result will explain the problem.
*
*
* @see #hasResolved()
* @see #getResolutionResult()
*/
Expand All @@ -282,12 +282,12 @@ public IProvisioningPlan getProvisioningPlan() {

/**
* Return the profile change request that describes the receiver.
*
*
* @return the profile change request. This may be <code>null</code> if the operation
* has not been resolved, or if a profile change request could not be assembled given
* the operation's state. If the profile change request is null and the operation has
* been resolved, the the resolution result will explain the problem.
*
*
* @see #hasResolved()
* @see #getResolutionResult()
* @since 2.1
Expand All @@ -299,22 +299,22 @@ public IProfileChangeRequest getProfileChangeRequest() {
}

/**
* Return a provisioning job that can be used to perform the resolved operation. The job is
* Return a provisioning job that can be used to perform the resolved operation. The job is
* created using the default values associated with a new job. It is up to clients to configure
* the priority of the job and set any appropriate properties, such as
* {@link Job#setUser(boolean)},
* {@link Job#setUser(boolean)},
* {@link Job#setSystem(boolean)}, or {@link Job#setProperty(QualifiedName, Object)},
* before scheduling it.
*
*
* @param monitor a progress monitor that should be used to report the job's progress in addition
* to the standard job progress reporting. Can be <code>null</code>. If provided, this monitor
* to the standard job progress reporting. Can be <code>null</code>. If provided, this monitor
* will be called from a background thread.
*
* @return a job that can be used to perform the provisioning operation. This may be <code>null</code>
*
* @return a job that can be used to perform the provisioning operation. This may be <code>null</code>
* if the operation has not been resolved, or if a plan could not be obtained when attempting to
* resolve. If the job is null and the operation has been resolved, then the resolution result
* resolve. If the job is null and the operation has been resolved, then the resolution result
* will explain the problem.
*
*
* @see #hasResolved()
* @see #getResolutionResult()
*/
Expand All @@ -337,7 +337,7 @@ public ProvisioningJob getProvisioningJob(IProgressMonitor monitor) {
* Set the provisioning context that should be used to resolve and perform the provisioning for
* the operation. This must be set before an attempt is made to resolve the operation
* for it to have any effect.
*
*
* @param context the provisioning context.
*/
public void setProvisioningContext(ProvisioningContext context) {
Expand All @@ -349,7 +349,7 @@ public void setProvisioningContext(ProvisioningContext context) {
/**
* Get the provisioning context that will be used to resolve and perform the provisioning for
* the operation.
*
*
* @return the provisioning context
*/
public ProvisioningContext getProvisioningContext() {
Expand All @@ -367,7 +367,7 @@ public String getProfileId() {
* change request, provisioning plan, or resolution result. It is possible that this
* method return <code>false</code> while resolution is taking place if it is performed
* in the background.
*
*
* @return <code>true</code> if the operation has been resolved, <code>false</code>
* if it has not resolved.
*/
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));
SubMonitor subMonitor = SubMonitor.convert(monitor, task, 1000);
status = getSession().performProvisioningPlan(plan, phaseSet, provisioningContext, subMonitor);
} finally {
monitor.done();
}
Expand Down
Loading

0 comments on commit e31c371

Please sign in to comment.