From b861462cc8824a423a1936f62b6fb96e00fda07c Mon Sep 17 00:00:00 2001 From: Patrick Ziegler Date: Thu, 19 Dec 2024 11:38:33 +0100 Subject: [PATCH] Ensure DownloadJob.run(IProgressMonitor) provides monitor task details IProgressMonitor.slice() only propagates the work to its parent monitor, but not any details. Follow-up to https://github.com/eclipse-equinox/p2/pull/576 --- .../repository/simple/DownloadJob.java | 80 ++++++++++++++++++- .../simple/SimpleArtifactRepository.java | 7 +- 2 files changed, 80 insertions(+), 7 deletions(-) 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..c977b19fda 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,6 +15,8 @@ package org.eclipse.equinox.internal.p2.artifact.repository.simple; import java.util.LinkedList; +import java.util.concurrent.locks.ReentrantLock; +import java.util.function.Supplier; import org.eclipse.core.runtime.*; import org.eclipse.core.runtime.jobs.Job; import org.eclipse.equinox.p2.repository.artifact.IArtifactRequest; @@ -24,11 +26,11 @@ public class DownloadJob extends Job { private final LinkedList requestsPending; private final SimpleArtifactRepository repository; - private final IProgressMonitor masterMonitor; + private final SubMonitor masterMonitor; private final MultiStatus overallStatus; DownloadJob(String name, SimpleArtifactRepository repository, LinkedList requestsPending, - IProgressMonitor masterMonitor, MultiStatus overallStatus) { + SubMonitor masterMonitor, MultiStatus overallStatus) { super(name); setSystem(true); this.repository = repository; @@ -56,8 +58,7 @@ protected IStatus run(IProgressMonitor jobMonitor) { 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); + IStatus status = repository.getArtifact(request, new ThreadSafeProgressMonitor(1)); if (!status.isOK()) { synchronized (overallStatus) { overallStatus.add(status); @@ -68,4 +69,75 @@ protected IStatus run(IProgressMonitor jobMonitor) { jobMonitor.done(); return Status.OK_STATUS; } + + /** + * Wrapper around the general {@link IProgressMonitor} to make it thread safe. + * All methods are wrapped within a {@link ReentrantLock} to ensure that only + * one {@link Thread} can notify the {@link #masterMonitor}. + */ + private class ThreadSafeProgressMonitor implements IProgressMonitor { + private static final ReentrantLock LOCK = new ReentrantLock(); + private final IProgressMonitor monitor; + + private ThreadSafeProgressMonitor(int ticks) { + this.monitor = masterMonitor.newChild(1); + } + + @Override + public void worked(int ticks) { + threadSafe(() -> monitor.worked(ticks)); + } + + @Override + public void internalWorked(double ticks) { + threadSafe(() -> monitor.internalWorked(ticks)); + } + + @Override + public void beginTask(String name, int totalWork) { + threadSafe(() -> monitor.beginTask(name, totalWork)); + } + + @Override + public void done() { + threadSafe(() -> monitor.done()); + } + + @Override + public void setCanceled(boolean value) { + threadSafe(() -> monitor.setCanceled(value)); + } + + @Override + public void setTaskName(String name) { + threadSafe(() -> monitor.setTaskName(name)); + } + + @Override + public void subTask(String name) { + threadSafe(() -> monitor.subTask(name)); + + } + + @Override + public boolean isCanceled() { + return threadSafe(() -> monitor.isCanceled()); + } + + private void threadSafe(Runnable runnable) { + threadSafe(() -> { + runnable.run(); + return null; + }); + } + + private T threadSafe(Supplier supplier) { + LOCK.lock(); + try { + return supplier.get(); + } finally { + LOCK.unlock(); + } + } + } } 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..2daf80dd7b 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 @@ -914,11 +914,12 @@ public IStatus getArtifacts(IArtifactRequest[] requests, IProgressMonitor monito } } else { // initialize the various jobs needed to process the get artifact requests - monitor.beginTask(NLS.bind(Messages.sar_downloading, Integer.toString(requests.length)), requests.length); + SubMonitor subMonitor = SubMonitor.convert(monitor, + NLS.bind(Messages.sar_downloading, Integer.toString(requests.length)), requests.length); try { DownloadJob jobs[] = new DownloadJob[numberOfJobs]; for (int i = 0; i < numberOfJobs; i++) { - jobs[i] = new DownloadJob(Messages.sar_downloadJobName + i, this, requestsPending, monitor, + jobs[i] = new DownloadJob(Messages.sar_downloadJobName + i, this, requestsPending, subMonitor, overallStatus); jobs[i].schedule(); } @@ -929,7 +930,7 @@ public IStatus getArtifacts(IArtifactRequest[] requests, IProgressMonitor monito //ignore } } finally { - monitor.done(); + subMonitor.done(); } }