Prevent endless download of artifact sources/javadoc (#252) #303
Conversation
+ fix total-work computation in classpath-update + minor clean up
| 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 |
There was a problem hiding this comment.
Could we start by something like !queue.contains(request) ? Wouldn't that already deliver a noticeable improvement without too much code complexity?
There was a problem hiding this comment.
Unfortunately that would not help in the case described in #252 (comment), because the new request is added when DownloadSourcesJob.updateClasspath() is called in DownloadSourcesJob.run(). But at that point the queue is already empty, so it would be added again.
That's the reason why the requests are cleared after updateClasspath() is called.
Alternatively we could either
- prevent
BuildPathManager.updateClasspath()from creating new requests (but this would probably require more flags and also adds complexity).
or
- simply clear the queue after
updateClasspath()returned inDownloadSourcesJob.run().
But, if any legitimate cases exists (do you know any?) whereupdateClasspath()creates follow-up downloads that downloads would be suppressed.
|
I admit I don't fully get the solution here; but since this has proven itself to be a fragile piece of code, it would be great if this bug could be covered by a test to prevent from future regression. Would that be doable? |
Fully agree. I'll try to set up some test-cases. |
I have created tests for this case and also other re-download cases and submitted them as a PR: tesla/m2e-core-tests#130 Without this PR the test BuildPathManagerTest.testDownloadSources_009_redownloadSnapshotWithOlderSources fails. .. just before in BuildPathManagerTest.testDownloadSources_009_redownloadSnapshotWithOlderSources the download of sources/javadoc is scheduled the second time: You will noticed that the break-point is hit indefinitely if the job is not cancelled (what the test case does after 10 round IIRC). When you look into the stack-trace you will find what I have described here: #252 (comment) |
|
This PR should not be merged up until #318 and tesla/m2e-core-tests#130 are merged. |
|
Great, thanks! Please rebase and merge when you're ready. |
|
Rebased and merged as 7a56eef ; thanks! |
|
Thanks for taking care of it. |
The changes of this PR prevent endless download loops of sources/javadoc which can still occur under special circumstances as described in #252 (comment).
The approach is to maintain a set of handled download requests, that guards against adding an equivalent request that is/was already handled in the current run of the DownloadSources-job.
Alternatively the queue could be cleaned in the end of each run, but this could suppress legitimate follow up download requests, or can the latter never happen?