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

ptziegler · 2024-12-19T10:42:00Z

IProgressMonitor.slice() only propagates the work to its parent monitor, but not any details.

Follow-up to #576

github-actions · 2024-12-19T10:44:37Z

Test Results

240 files - 135 240 suites - 135 29m 1s ⏱️ - 17m 27s
1 898 tests - 6 1 895 ✅ - 6 3 💤 ±0 0 ❌ ±0
4 350 runs - 2 362 4 347 ✅ - 2 356 3 💤 - 6 0 ❌ ±0

Results for commit b861462. ± Comparison against base commit 1cfa5de.

This pull request removes 6 tests.

AutomatedTests org.eclipse.equinox.p2.tests.touchpoint.natives.AllTests org.eclipse.equinox.p2.tests.touchpoint.natives.CheckAndPromptNativePackageWindowsRegistryTest ‑ execute_IntAttribute
AutomatedTests org.eclipse.equinox.p2.tests.touchpoint.natives.AllTests org.eclipse.equinox.p2.tests.touchpoint.natives.CheckAndPromptNativePackageWindowsRegistryTest ‑ execute_IntAttribute_DifferentLiteralValue
AutomatedTests org.eclipse.equinox.p2.tests.touchpoint.natives.AllTests org.eclipse.equinox.p2.tests.touchpoint.natives.CheckAndPromptNativePackageWindowsRegistryTest ‑ execute_KeyExistence
AutomatedTests org.eclipse.equinox.p2.tests.touchpoint.natives.AllTests org.eclipse.equinox.p2.tests.touchpoint.natives.CheckAndPromptNativePackageWindowsRegistryTest ‑ execute_KeyExistence_DifferentKeys
AutomatedTests org.eclipse.equinox.p2.tests.touchpoint.natives.AllTests org.eclipse.equinox.p2.tests.touchpoint.natives.CheckAndPromptNativePackageWindowsRegistryTest ‑ execute_StringAttribute
AutomatedTests org.eclipse.equinox.p2.tests.touchpoint.natives.AllTests org.eclipse.equinox.p2.tests.touchpoint.natives.CheckAndPromptNativePackageWindowsRegistryTest ‑ execute_StringAttribute_DifferentValues

♻️ This comment has been updated with latest results.

vogella · 2024-12-19T10:58:53Z

Not sure what is causing this, maybe JDT is re-deploying?

org.apache.maven.InternalErrorException: Internal error: java.lang.RuntimeException: org.apache.maven.plugin.PluginResolutionException: Plugin org.eclipse.tycho:tycho-compiler-plugin:4.0.10 or one of its dependencies could not be resolved:
Could not find artifact org.eclipse.jdt:ecj:jar:3.41.0-SNAPSHOT in eclipse-snapshots (https://repo.eclipse.org/content/repositories/eclipse-snapshots/)

vogella · 2024-12-19T11:04:18Z

Reported here eclipse-platform/eclipse.platform.swt#1683

ptziegler · 2024-12-19T11:06:44Z

But Equinox is using Tycho 4.0.10... Why is a release depending on a snapshot? That doesn't look right to me.

merks · 2024-12-19T11:11:15Z

@ptziegler

This is cleaner indeed and also fixes the problem. (With Oomph one can combine the project setup for p2 and Oomph itself to have a development environment where I can easy test changes to both.)

It looks like some build infrastructure problems kicked in...

merks · 2024-12-19T11:16:01Z

FYI, this version of ecj was just build a short while ago:

https://repo.eclipse.org/content/repositories/eclipse-snapshots/org/eclipse/jdt/ecj/3.41.0-SNAPSHOT/

So while the build says it can't be found, you can see that it really does exist there...

merks · 2024-12-19T11:24:32Z

Don't bother forcing new builds until the infrastructure problem is fixed.

Thanks for your efforts so far!

I will push appropriate buttons to restart builds when things look to be in better shape later in the day (I hope).

ptziegler · 2024-12-19T11:26:41Z

Don't bother forcing new builds until the infrastructure problem is fixed.

I simply noticed that line 933 was still referencing monitor, not subMonitor, which I just updated. 😅

merks · 2024-12-19T11:27:23Z

Oh. Fixing is allowed. 🤣

And I retested and it still works.

merks · 2024-12-19T11:57:54Z

I created this issue:

https://gitlab.eclipse.org/eclipsefdn/helpdesk/-/issues/5454

laeubi · 2024-12-19T13:03:43Z

...t.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/simple/DownloadJob.java

@@ -56,8 +56,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, masterMonitor.newChild(1));


Please note that submonitor is not threadsafe! Also now the job monitor do not recive any updates.

It might therefore be better to pass in an own implementation that:

syncronize on the master before update it

also send all events to the job monitor

I'm trying to understand the nature of the comment. Even before the change that introduced problem (call to slice) it was a submonitor already then too:

Was your comment intended for the change in the other file?

https://github.com/eclipse-equinox/p2/pull/586/files#diff-9a34bd9b50e863d2faa5988b40185db6a3a7e144ef93175808e9e44c19394840

Note that I did test that I see logged information in the installer with these changes, so I'm not sure why the job monitor is not receiving updates...

Maybe we shouldn't be changing this at all? It's very surprising to me that every change has a different gotcha. It's still not clear if anything was actually improved...

Note that the code I changed was a botched migration to the SubMonitor, which started this whole ordeal. Originally, the now deprecated SubProgressMonitor was used.

9009085#diff-98743f36a52a7f919e0b5cb2ce651e1a89f1f3a7abf1c343e58a29ee9340e2b5

syncronize on the master before update it

Without synchronization, some ticks might indeed get lost due to parallelism. But I wonder if this is really worth the additional effort... done() is explicitly called on the SubMonitor, so any remaining ticks will be consumed once the download is done, so it's at least not a critical problem.

also send all events to the job monitor

Wouldn't this effectively mean both jobs show the same task?
Edit: And just to clarify: The jobMonitor has never received any events.

Is the assertion "The subMonitor created in the run() method is only used on this job's thread, i.e., on the thread that is calling run()" a true assertion? I thought that was the case, but I've been known to be wrong.

I would have though only the thread safety of the masterMonitor is a potential concern...

@ptziegler has correctly explained it sorry for being late.

One should also keep in mind that newChild/split clear the work acquired by a previous call, so if you do:

IProgressMonitor m1 = monitor.split(10); IProgressMonitor m2 = monitor.split(10);

then you have one Monitor (m1) that is already done, so only the last job started could report actual progress!

Reporting good and consistent progress is a very hard topic indeed and Eclipse makes its even harder some times due to historical reasons.

The "slice" was introduced by me a while ago specifically for the case where one do work in parallel Threads (usually streams or jobs) that still perform a common task but want to report overall progress. Adapted to this situation here for example one would have one (of unknown origin) Monitor and set its task to "Downloading files", and the amount of work to the number of files. Now I start a thread for each file and pass it a slice of one and this thread can now use safely the monitor (but needs to call done possibly) to report its overall download progress while everything is aggregated into the original one, that might observe the number of running Threads and update the message (e.g. with "Downloading files 5 / 10").

Another reason for slice is to notify the main monitor about that progress was distributed (what is impossible with SubMonitor at the moment) so an UI can show something like this:

Image from here

IProgressMonitor.slice() only propagates the work to its parent monitor, but not any details. Follow-up to eclipse-equinox#576

ptziegler · 2024-12-19T23:13:42Z

I've created a thread-safe implementation of the IProgressMonitor which should synchronize with the master monitor. As mentioned, the jobMonitor is not notified as this was never the case before.

laeubi · 2024-12-20T05:35:50Z

...t.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/simple/DownloadJob.java

@@ -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));


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

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

merks · 2024-12-20T06:24:31Z

...src/org/eclipse/equinox/internal/p2/artifact/repository/simple/SimpleArtifactRepository.java

 			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,


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

laeubi · 2024-12-20T07:41:26Z

@ptziegler @merks I have now created a PR that I think solves the problem more generally and has the advantage that individual download jobs report proper progress and messages:

Improve progress reporting for download jobs #587

merks · 2024-12-27T07:23:24Z

Superseded by #587

ptziegler mentioned this pull request Dec 19, 2024

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

Closed

ptziegler force-pushed the download-job-details branch from 57a3cf3 to 25a0287 Compare December 19, 2024 11:21

laeubi reviewed Dec 19, 2024

View reviewed changes

Ensure DownloadJob.run(IProgressMonitor) provides monitor task details

b861462

IProgressMonitor.slice() only propagates the work to its parent monitor, but not any details. Follow-up to eclipse-equinox#576

ptziegler force-pushed the download-job-details branch from 25a0287 to b861462 Compare December 19, 2024 23:11

laeubi reviewed Dec 20, 2024

View reviewed changes

merks reviewed Dec 20, 2024

View reviewed changes

merks closed this Dec 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ptziegler commented Dec 19, 2024

github-actions bot commented Dec 19, 2024 •

edited

Loading

vogella commented Dec 19, 2024

vogella commented Dec 19, 2024

ptziegler commented Dec 19, 2024

merks commented Dec 19, 2024

merks commented Dec 19, 2024

merks commented Dec 19, 2024

ptziegler commented Dec 19, 2024

merks commented Dec 19, 2024 •

edited

Loading

merks commented Dec 19, 2024

laeubi Dec 19, 2024

merks Dec 19, 2024

ptziegler Dec 19, 2024 •

edited

Loading

merks Dec 19, 2024 •

edited

Loading

laeubi Dec 20, 2024

ptziegler commented Dec 19, 2024

laeubi Dec 20, 2024

merks Dec 20, 2024

merks Dec 20, 2024

laeubi commented Dec 20, 2024

merks commented Dec 27, 2024

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

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

Conversation

ptziegler commented Dec 19, 2024

github-actions bot commented Dec 19, 2024 • edited Loading

Test Results

vogella commented Dec 19, 2024

vogella commented Dec 19, 2024

ptziegler commented Dec 19, 2024

merks commented Dec 19, 2024

merks commented Dec 19, 2024

merks commented Dec 19, 2024

ptziegler commented Dec 19, 2024

merks commented Dec 19, 2024 • edited Loading

merks commented Dec 19, 2024

laeubi Dec 19, 2024

Choose a reason for hiding this comment

merks Dec 19, 2024

Choose a reason for hiding this comment

ptziegler Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

merks Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

laeubi Dec 20, 2024

Choose a reason for hiding this comment

ptziegler commented Dec 19, 2024

laeubi Dec 20, 2024

Choose a reason for hiding this comment

merks Dec 20, 2024

Choose a reason for hiding this comment

merks Dec 20, 2024

Choose a reason for hiding this comment

laeubi commented Dec 20, 2024

merks commented Dec 27, 2024

github-actions bot commented Dec 19, 2024 •

edited

Loading

merks commented Dec 19, 2024 •

edited

Loading

ptziegler Dec 19, 2024 •

edited

Loading

merks Dec 19, 2024 •

edited

Loading