Fix IProgressMonitor usage eclipse-equinox#335

passing a monitor to two callees requires it to be splitted. eclipse-equinox#335 Also fixed forgotten monitor.done() in performProvisioningPlan and improved tests.
jukzi · Oct 29, 2023 · 25832ef · 25832ef
1 parent c4fc651
commit 25832ef
Show file tree

Hide file tree

Showing 12 changed files with 51 additions and 45 deletions.
diff --git a/....repository/src/org/eclipse/equinox/internal/p2/artifact/repository/RawMirrorRequest.java b/....repository/src/org/eclipse/equinox/internal/p2/artifact/repository/RawMirrorRequest.java
@@ -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) {
@@ -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());
@@ -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));
 	}
 }
diff --git a/.../org/eclipse/equinox/internal/p2/artifact/repository/simple/SimpleArtifactRepository.java b/.../org/eclipse/equinox/internal/p2/artifact/repository/simple/SimpleArtifactRepository.java
@@ -614,7 +614,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) {
@@ -652,8 +652,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

diff --git a/bundles/org.eclipse.equinox.p2.engine/src/org/eclipse/equinox/internal/p2/engine/Engine.java b/bundles/org.eclipse.equinox.p2.engine/src/org/eclipse/equinox/internal/p2/engine/Engine.java
@@ -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)
@@ -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));
@@ -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));
@@ -121,6 +122,7 @@ public IStatus perform(IProfile iprofile, IPhaseSet phases, Operand[] operands,
 		} finally {
 			profileRegistry.unlockProfile(profile);
 			profile.setChanged(false);
+			monitor.done();
 		}
 	}
 

diff --git a/...e.equinox.p2.operations/src/org/eclipse/equinox/p2/operations/ProfileChangeOperation.java b/...e.equinox.p2.operations/src/org/eclipse/equinox/p2/operations/ProfileChangeOperation.java
@@ -110,12 +110,11 @@ protected ProfileChangeOperation(ProvisioningSession session) {
 	 * @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;
 		}

diff --git a/...e.equinox.p2.operations/src/org/eclipse/equinox/p2/operations/ProfileModificationJob.java b/...e.equinox.p2.operations/src/org/eclipse/equinox/p2/operations/ProfileModificationJob.java
@@ -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();
 		}

diff --git a/...ipse.equinox.p2.operations/src/org/eclipse/equinox/p2/operations/ProvisioningSession.java b/...ipse.equinox.p2.operations/src/org/eclipse/equinox/p2/operations/ProvisioningSession.java
@@ -21,7 +21,6 @@
 import org.eclipse.core.runtime.*;
 import org.eclipse.core.runtime.jobs.*;
 import org.eclipse.equinox.internal.p2.core.helpers.ServiceHelper;
-import org.eclipse.equinox.internal.p2.operations.Constants;
 import org.eclipse.equinox.internal.p2.operations.Messages;
 import org.eclipse.equinox.internal.provisional.configurator.Configurator;
 import org.eclipse.equinox.internal.provisional.p2.core.eventbus.IProvisioningEventBus;
@@ -155,15 +154,13 @@ public IStatus performProvisioningPlan(IProvisioningPlan plan, IPhaseSet phaseSe
 				IPhaseSet download = PhaseSetFactory.createPhaseSetIncluding(new String[] {PhaseSetFactory.PHASE_COLLECT});
 				IStatus downloadStatus = getEngine().perform(downloadPlan, download, mon.newChild(300));
 				if (!downloadStatus.isOK()) {
-					mon.done();
 					return downloadStatus;
 				}
 				ticksUsed = 300;
 			}
 			// we pre-downloaded if necessary.  Now perform the install plan against the original phase set.
 			IStatus installerPlanStatus = getEngine().perform(plan.getInstallerPlan(), set, mon.newChild(100));
 			if (!installerPlanStatus.isOK()) {
-				mon.done();
 				return installerPlanStatus;
 			}
 			ticksUsed += 100;
@@ -173,8 +170,7 @@ public IStatus performProvisioningPlan(IProvisioningPlan plan, IPhaseSet phaseSe
 			try {
 				configChanger.applyConfiguration();
 			} catch (IOException e) {
-				mon.done();
-				return new Status(IStatus.ERROR, Constants.BUNDLE_ID, Messages.ProvisioningSession_InstallPlanConfigurationError, e);
+				return Status.error(Messages.ProvisioningSession_InstallPlanConfigurationError, e);
 			}
 		}
 		return getEngine().perform(plan, set, mon.newChild(500 - ticksUsed));

diff --git a/...e.equinox.p2.tests.ui/src/org/eclipse/equinox/p2/tests/ui/AbstractProvisioningUITest.java b/...e.equinox.p2.tests.ui/src/org/eclipse/equinox/p2/tests/ui/AbstractProvisioningUITest.java
@@ -18,6 +18,7 @@
 import java.util.Dictionary;
 import java.util.Hashtable;
 import org.eclipse.core.runtime.*;
+import org.eclipse.core.tests.harness.FussyProgressMonitor;
 import org.eclipse.equinox.internal.p2.director.ProfileChangeRequest;
 import org.eclipse.equinox.internal.p2.ui.model.ProfileElement;
 import org.eclipse.equinox.internal.p2.ui.sdk.SimpleLicenseManager;
@@ -146,12 +147,19 @@ protected IStatus install(IInstallableUnit iu, boolean root, boolean lock) {
 		// Use an empty provisioning context to prevent repo access
 		ProvisioningContext context = new ProvisioningContext(getAgent());
 		context.setMetadataRepositories(new URI[] {});
+		FussyProgressMonitor monitor = new FussyProgressMonitor();
 		IProvisioningPlan plan = getPlanner(getSession().getProvisioningAgent()).getProvisioningPlan(req, context,
-				getMonitor());
+				monitor);
+		monitor.assertUsedUp();
 		if (plan.getStatus().getSeverity() == IStatus.ERROR || plan.getStatus().getSeverity() == IStatus.CANCEL)
 			return plan.getStatus();
-		return getSession().performProvisioningPlan(plan, PhaseSetFactory.createDefaultPhaseSet(),
-				new ProvisioningContext(getAgent()), getMonitor());
+		FussyProgressMonitor monitor2 = new FussyProgressMonitor();
+		try {
+			return getSession().performProvisioningPlan(plan, PhaseSetFactory.createDefaultPhaseSet(),
+					new ProvisioningContext(getAgent()), monitor2);
+		} finally {
+			monitor2.assertUsedUp();
+		}
 	}
 
 	protected IInstallableUnit createNamedIU(String id, String name, Version version, boolean isCategory) {

diff --git a/...pse.equinox.p2.tests.ui/src/org/eclipse/equinox/p2/tests/ui/dialogs/UpdateWizardTest.java b/...pse.equinox.p2.tests.ui/src/org/eclipse/equinox/p2/tests/ui/dialogs/UpdateWizardTest.java
@@ -14,6 +14,7 @@
 package org.eclipse.equinox.p2.tests.ui.dialogs;
 
 import java.util.ArrayList;
+import org.eclipse.core.tests.harness.FussyProgressMonitor;
 import org.eclipse.equinox.internal.p2.metadata.License;
 import org.eclipse.equinox.internal.p2.ui.ProvUI;
 import org.eclipse.equinox.internal.p2.ui.dialogs.*;
@@ -183,7 +184,9 @@ public void testBug277554MultipleVersions() {
 		ArrayList<IInstallableUnit> iusInvolved = new ArrayList<>();
 		iusInvolved.add(main);
 		UpdateOperation op = getProvisioningUI().getUpdateOperation(iusInvolved, null);
-		op.resolveModal(getMonitor());
+		FussyProgressMonitor monitor = new FussyProgressMonitor();
+		op.resolveModal(monitor);
+		monitor.assertUsedUp();
 		UpdateWizard wizard = new UpdateWizard(getProvisioningUI(), op, op.getSelectedUpdates(), null);
 		ProvisioningWizardDialog dialog = new ProvisioningWizardDialog(ProvUI.getDefaultParentShell(), wizard);
 		dialog.setBlockOnOpen(false);

diff --git a/...g.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/AbstractProvisioningTest.java b/...g.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/AbstractProvisioningTest.java
@@ -55,9 +55,9 @@
 import org.eclipse.core.runtime.IPath;
 import org.eclipse.core.runtime.IProgressMonitor;
 import org.eclipse.core.runtime.IStatus;
-import org.eclipse.core.runtime.NullProgressMonitor;
 import org.eclipse.core.runtime.Platform;
 import org.eclipse.core.runtime.URIUtil;
+import org.eclipse.core.tests.harness.FussyProgressMonitor;
 import org.eclipse.equinox.internal.p2.artifact.repository.simple.SimpleArtifactRepository;
 import org.eclipse.equinox.internal.p2.core.helpers.ServiceHelper;
 import org.eclipse.equinox.internal.p2.core.helpers.URLUtil;
@@ -842,7 +842,7 @@ public IProfile createProfile(String name, Map<String, String> properties) {
 	}
 
 	protected IProgressMonitor getMonitor() {
-		return new NullProgressMonitor();
+		return new FussyProgressMonitor();
 	}
 
 	protected IProfile getProfile(String profileId) {

diff --git a/...export/src/org/eclipse/equinox/internal/p2/importexport/internal/wizard/ImportWizard.java b/...export/src/org/eclipse/equinox/internal/p2/importexport/internal/wizard/ImportWizard.java
@@ -96,8 +96,8 @@ public void recomputePlan(IRunnableContext runnableContext, final boolean withRe
 		if (((ImportPage) mainPage).hasUnloadedRepo()) {
 			try {
 				runnableContext.run(true, true, monitor -> {
-					final SubMonitor sub = SubMonitor.convert(monitor, 1000);
-					((ImportPage) mainPage).recompute(sub.newChild(800));
+					final SubMonitor sub = SubMonitor.convert(monitor, withRemediation ? 15 : 10);
+					((ImportPage) mainPage).recompute(sub.newChild(8));
 					if (sub.isCanceled())
 						throw new InterruptedException();
 					Display.getDefault().syncExec(() -> {
@@ -127,12 +127,12 @@ public IStatus getResolutionResult() {
 					});
 					if (sub.isCanceled())
 						throw new InterruptedException();
-					if (operation.resolveModal(sub.newChild(200)).getSeverity() == IStatus.CANCEL)
+					if (operation.resolveModal(sub.newChild(2)).getSeverity() == IStatus.CANCEL)
 						throw new InterruptedException();
 					if (withRemediation) {
 						IStatus status = operation.getResolutionResult();
 						if (remediationPage != null && shouldRemediate(status)) {
-							computeRemediationOperation(operation, ui, monitor);
+							computeRemediationOperation(operation, ui, sub.newChild(5));
 						}
 					}
 					Display.getDefault().asyncExec(this::planChanged);

diff --git a/...nox.p2.ui/src/org/eclipse/equinox/internal/p2/ui/dialogs/ProvisioningOperationWizard.java b/...nox.p2.ui/src/org/eclipse/equinox/internal/p2/ui/dialogs/ProvisioningOperationWizard.java
@@ -27,7 +27,8 @@
 import org.eclipse.equinox.p2.engine.IProvisioningPlan;
 import org.eclipse.equinox.p2.engine.ProvisioningContext;
 import org.eclipse.equinox.p2.metadata.IInstallableUnit;
-import org.eclipse.equinox.p2.operations.*;
+import org.eclipse.equinox.p2.operations.ProfileChangeOperation;
+import org.eclipse.equinox.p2.operations.RemediationOperation;
 import org.eclipse.equinox.p2.ui.*;
 import org.eclipse.jface.operation.IRunnableContext;
 import org.eclipse.jface.wizard.*;
@@ -311,12 +312,10 @@ public void recomputePlan(IRunnableContext runnableContext) {
 	}
 
 	public void computeRemediationOperation(ProfileChangeOperation op, ProvisioningUI ui, IProgressMonitor monitor) {
-		SubMonitor sub = SubMonitor.convert(monitor, ProvUIMessages.ProvisioningOperationWizard_Remediation_Operation,
-				RemedyConfig.getAllRemedyConfigs().length);
 		monitor.setTaskName(ProvUIMessages.ProvisioningOperationWizard_Remediation_Operation);
 		remediationOperation = new RemediationOperation(ui.getSession(), op.getProfileChangeRequest());
 		remediationOperation.resolveModal(monitor);
-		sub.done();
+		monitor.done();
 	}
 
 	/**

diff --git a/...les.rcp.prestartupdate/src/org/eclipse/equinox/p2/examples/rcp/prestartupdate/P2Util.java b/...les.rcp.prestartupdate/src/org/eclipse/equinox/p2/examples/rcp/prestartupdate/P2Util.java
@@ -47,9 +47,8 @@ static IStatus checkForUpdates(IProvisioningAgent agent, IProgressMonitor monito
 		// which installable units are being updated, use the more detailed
 		// constructors.
 		UpdateOperation operation = new UpdateOperation(session);
-		SubMonitor sub = SubMonitor.convert(monitor,
-				"Checking for application updates...", 200);
-		IStatus status = operation.resolveModal(sub.newChild(100));
+		SubMonitor sub = SubMonitor.convert(monitor, "Checking for application updates...", 2);
+		IStatus status = operation.resolveModal(sub.split(1));
 		if (status.getCode() == UpdateOperation.STATUS_NOTHING_TO_UPDATE) {
 			return status;
 		}
@@ -61,9 +60,8 @@ static IStatus checkForUpdates(IProvisioningAgent agent, IProgressMonitor monito
 			// are available if there are multiples, differentiating patches vs. updates, etc.
 			// In this example, we simply update as suggested by the operation.
 			ProvisioningJob job = operation.getProvisioningJob(null);
-			status = job.runModal(sub.newChild(100));
-			if (status.getSeverity() == IStatus.CANCEL)
-				throw new OperationCanceledException();
+			status = job.runModal(sub.split(1));
+			sub.checkCanceled();
 		}
 		return status;
 	}