Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure DownloadJob.run(IProgressMonitor) provides monitor task details #586

Closed
wants to merge 1 commit into from
Closed

Ensure DownloadJob.run(IProgressMonitor) provides monitor task details #586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 76 additions & 4 deletions ...epository/src/org/eclipse/equinox/internal/p2/artifact/repository/simple/DownloadJob.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -24,11 +26,11 @@ public class DownloadJob extends Job {

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

DownloadJob(String name, SimpleArtifactRepository repository, LinkedList<IArtifactRequest> requestsPending,
IProgressMonitor masterMonitor, MultiStatus overallStatus) {
SubMonitor masterMonitor, MultiStatus overallStatus) {
super(name);
setSystem(true);
this.repository = repository;
Expand Down Expand Up @@ -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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I though that there are multiple jobs, so doe not need the "master monitor" already be thread safe?!?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is why I asked the question I asked yesterday. The monitor here is used only in this thread I believe.

if (!status.isOK()) {
synchronized (overallStatus) {
overallStatus.add(status);
Expand All @@ -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> T threadSafe(Supplier<T> supplier) {
LOCK.lock();
try {
return supplier.get();
} finally {
LOCK.unlock();
}
}
}
}
7 changes: 4 additions & 3 deletions .../org/eclipse/equinox/internal/p2/artifact/repository/simple/SimpleArtifactRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand it, the master monitor is now created here and has become a submonitor. So it this a/the threading concern?

overallStatus);
jobs[i].schedule();
}
Expand All @@ -929,7 +930,7 @@ public IStatus getArtifacts(IArtifactRequest[] requests, IProgressMonitor monito
//ignore
}
} finally {
monitor.done();
subMonitor.done();
}
}

Expand Down
Loading