Prevent endless download of artifact sources/javadoc (eclipse-m2e#252)

+ fix total-work computation in classpath-update + minor clean up
HannesWell · Aug 21, 2021 · 19605fa · 19605fa
1 parent 0fd414c
commit 19605fa
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 33 deletions.
diff --git a/org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/BuildPathManager.java b/org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/BuildPathManager.java
@@ -332,23 +332,24 @@ private void configureAttachedSourcesAndJavadoc(IMavenProjectFacade facade, Prop
         if(aKey != null) { // maybe we should try to find artifactKey little harder here?
           boolean isSnapshot = aKey.getVersion().endsWith("-SNAPSHOT");
           // We should update a sources/javadoc jar for a snapshot in case they're already downloaded.
-          File plainFile = desc.getPath() != null ? desc.getPath().toFile() : null;
+          File jarFile = desc.getPath() != null ? desc.getPath().toFile() : null;
           File srcFile = srcPath != null ? srcPath.toFile() : null;
           boolean downloadSources = (srcPath == null && mavenConfiguration.isDownloadSources())
-              || (plainFile != null && plainFile.canRead() && srcFile != null && srcFile.canRead() && isSnapshot
-                  && srcFile.lastModified() < plainFile.lastModified());
+              || (isSnapshot && isLastModifiedBefore(srcFile, jarFile));
           File javaDocFile = javaDocUrl != null ? getAttachedArtifactFile(aKey, CLASSIFIER_JAVADOC) : null;
           boolean downloadJavaDoc = (javaDocUrl == null && mavenConfiguration.isDownloadJavaDoc())
-              || (plainFile != null && plainFile.canRead() && javaDocFile != null && javaDocFile.canRead() && isSnapshot
-                  && javaDocFile.lastModified() < plainFile.lastModified());
-
+              || (isSnapshot && isLastModifiedBefore(javaDocFile, jarFile));
           scheduleDownload(facade.getProject(), facade.getMavenProject(monitor), aKey, downloadSources,
               downloadJavaDoc);
         }
       }
     }
   }
 
+  private static boolean isLastModifiedBefore(File file, File ref) {
+    return ref != null && ref.canRead() && file != null && file.canRead() && file.lastModified() < ref.lastModified();
+  }
+
   private static final String ARTIFACT_TYPE_JAR = "jar";
 
   private boolean isUnavailable(ArtifactKey a, List<ArtifactRepository> repositories) throws CoreException {

diff --git a/org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/DownloadSourcesJob.java b/org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/DownloadSourcesJob.java
@@ -14,7 +14,6 @@
 package org.eclipse.m2e.jdt.internal;
 
 import java.io.File;
-import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -58,6 +57,7 @@
  *
  * @author igor
  */
+@SuppressWarnings("restriction")
 class DownloadSourcesJob extends Job implements IBackgroundProcessingQueue {
   private static Logger log = LoggerFactory.getLogger(DownloadSourcesJob.class);
 
@@ -137,6 +137,8 @@ public boolean isNotEmpty() {
 
   private final BlockingQueue<DownloadRequest> queue = new LinkedBlockingQueue<>();
 
+  private Set<DownloadRequest> requests = new HashSet<>();
+
   private final Set<IProject> toUpdateMavenProjects = new HashSet<>();
 
   private final Map<IPackageFragmentRoot, Attachments> toUpdateAttachments = new HashMap<>();
@@ -163,6 +165,7 @@ public IStatus run(IProgressMonitor monitor) {
         if(!status.isOK()) {
           // or maybe just log and ignore?
           queue.clear();
+          requests.clear();
           toUpdateAttachments.clear();
           toUpdateMavenProjects.clear();
           return status;
@@ -173,6 +176,7 @@ public IStatus run(IProgressMonitor monitor) {
     }
     if(monitor.isCanceled()) {
       queue.clear();
+      requests.clear();
       toUpdateAttachments.clear();
       toUpdateMavenProjects.clear();
       return Status.CANCEL_STATUS;
@@ -186,18 +190,16 @@ public IStatus run(IProgressMonitor monitor) {
       toUpdateAttachments.clear();
       toUpdateMavenProjects.clear();
     }
+    requests.clear(); // retain all elements in queue (in an efficient manner)
+    requests.addAll(queue); // queue might not be empty anymore (filled by updateClasspath)
     subMonitor.done();
-    if(monitor.isCanceled()) {
-      return Status.CANCEL_STATUS;
-    }
-    return Status.OK_STATUS;
+    return monitor.isCanceled() ? Status.CANCEL_STATUS : Status.OK_STATUS;
   }
 
   private static void updateClasspath(BuildPathManager manager, Set<IProject> toUpdateMavenProjects,
       Map<IPackageFragmentRoot, Attachments> toUpdateAttachments, IProgressMonitor monitor) {
-    SubMonitor updateMonitor = SubMonitor.convert(monitor,
-        1 + toUpdateMavenProjects.size() + toUpdateMavenProjects.size());
-    updateMonitor.setTaskName(Messages.DownloadSourcesJob_job_associateWithClasspath);
+    SubMonitor updateMonitor = SubMonitor.convert(monitor, Messages.DownloadSourcesJob_job_associateWithClasspath,
+        1 + toUpdateMavenProjects.size() + toUpdateAttachments.size());
     ISchedulingRule schedulingRule = ResourcesPlugin.getWorkspace().getRuleFactory().buildRule();
     getJobManager().beginRule(schedulingRule, updateMonitor.split(1));
     try {
@@ -219,8 +221,6 @@ private static void updateClasspath(BuildPathManager manager, Set<IProject> toUp
   }
 
   IStatus downloadFilesAndPopulateToUpdate(DownloadRequest request, IProgressMonitor monitor) {
-    final List<IStatus> exceptions = new ArrayList<>();
-
     SubMonitor requestMonitor = SubMonitor.convert(monitor, 33);
     try {
       if(request.artifact != null) {
@@ -247,17 +247,13 @@ IStatus downloadFilesAndPopulateToUpdate(DownloadRequest request, IProgressMonit
           toUpdateAttachments.put(request.fragment, files);
         }
       }
+      return Status.OK_STATUS;
     } catch(CoreException ex) {
-      exceptions.add(ex.getStatus());
-    }
-    requestMonitor.done();
-
-    if(!exceptions.isEmpty()) {
-      IStatus[] problems = exceptions.toArray(new IStatus[exceptions.size()]);
-      return new MultiStatus(MavenJdtPlugin.PLUGIN_ID, -1, problems, "Could not download sources or javadoc", null);
+      return new MultiStatus(MavenJdtPlugin.PLUGIN_ID, -1, new IStatus[] {ex.getStatus()},
+          "Could not download sources or javadoc", null);
+    } finally {
+      requestMonitor.done();
     }
-
-    return Status.OK_STATUS;
   }
 
   private Attachments downloadMaven(IMavenProjectFacade projectFacade, ArtifactKey artifact, boolean downloadSources,
@@ -339,17 +335,14 @@ private File download(ArtifactKey artifact, List<ArtifactRepository> repositorie
 
   private void scheduleDownload(IProject project, IPackageFragmentRoot fragment, ArtifactKey artifact,
       boolean downloadSources, boolean downloadJavadoc) {
-    addDownloadRequest(project, fragment, artifact, downloadSources, downloadJavadoc);
-
-    schedule(SCHEDULE_INTERVAL);
-  }
-
-  public void addDownloadRequest(IProject project, IPackageFragmentRoot fragment, ArtifactKey artifact,
-      boolean downloadSources, boolean downloadJavadoc) {
     if(project == null || !project.isAccessible()) {
       return;
     }
-    queue.add(new DownloadRequest(project, fragment, artifact, downloadSources, downloadJavadoc));
+    DownloadRequest request = new DownloadRequest(project, fragment, artifact, downloadSources, downloadJavadoc);
+    if(requests.add(request)) { // guard against new requests that are/will be already downloaded in this run to prevent endless loops
+      queue.add(request);
+      schedule(SCHEDULE_INTERVAL);
+    }
   }
 
   /**