Correct improper usage of the SubMonitor #576
Conversation
|
I noticed that the target definition resolution usually gets stuck at 47%, which I believe is partially caused by a lot of funny stuff being done to the progress monitor when executing the download job...
|
There was a problem hiding this comment.
I'm just looking via my phone but I assume that the done() call (and finally block) can be removed. See https://www.eclipse.org/articles/Article-Progress-Monitors/article.html
|
See section 5.7 |
|
This pull request changes some projects for the first time in this development cycle. An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch. Git patchFurther information are available in Common Build Issues - Missing version increments. |
ptziegler
force-pushed
the
sub-monitor
branch
2 times, most recently
from
b842ec3 to
353f88e
Compare
Technically yes, but I think my mistake was that the call to done() still needs to be done for the main monitor. Meaning the calls in the finally block should remain
|
| try { | ||
| for (AbstractDiscoveryStrategy discoveryStrategy : discoveryStrategies) { | ||
| if (monitor.isCanceled()) { |
There was a problem hiding this comment.
Is this extra check necessary? We do a split (which auto checks for cancellations ) just a few lines later
There was a problem hiding this comment.
Is this extra check necessary? We do a split (which auto checks for cancellations ) just a few lines later
This is not completely the same. In one case you can silently return in the other an exception is thrown.
There was a problem hiding this comment.
Split also throws this exception
There was a problem hiding this comment.
That's true. But given that the method is supposed to return gracefully upon cancellation, rather than throwing an exception, I should use newChild() rather than split().
| IPlanner planner = ui.getSession().getProvisioningAgent().getService(IPlanner.class); | ||
| try { | ||
| Set<IInstallableUnit> allUpdates = new HashSet<>(); | ||
| for (IInstallableUnit unit : iusToUpdate) { | ||
| if (monitor.isCanceled()) | ||
| if (subMonitor.isCanceled()) |
There was a problem hiding this comment.
This whole if check can go, given that the OperationCanceledException is explicitly caught. In both cases, Collector.emptyCollector() is returned.
I think this refers to the "top most caller" that is the one that created the monitor / acquired it for the first time. For a Job I would expect that the job framework is calling |
|
Yes to @laeubi statement of the done call. If you don't create the monitor you do not have to call done |
|
@ptziegler we have also tracing to find incorrect usage of progress monitoring, see the article (search for tracing). Years ago @kthoms and I a spend lot of time trying to clean the usage up in platform and pde but there were to many to fix them all . |
ptziegler
force-pushed
the
sub-monitor
branch
from
353f88e to
5917366
Compare
|
I've removed the try-finally blocks that only serve to call
Looks interesting. Perhaps I can give it a go later on. It wouldn't surprise me if there are other places where the SubProgressMonitor was replaced too eagerly. |
ptziegler
force-pushed
the
sub-monitor
branch
from
5917366 to
3871c96
Compare
|
@ptziegler is this ready to be merged from your side? |
Yes. From my side, I'm done. |
> Since it is illegal to call beginTask on the same IProgressMonitor > more than once, the same instance of IProgressMonitor must not be > passed to convert more than once. The proper way is to replace calls to IProgressMonitor.beginTask(...) with SubMonitor.convert(...), keep the SubMonitor as a local variable and use that from now on. This amends commit 9009085.
ptziegler
force-pushed
the
sub-monitor
branch
from
3871c96 to
b131c75
Compare
|
Build works fine with GH actions. The Jenkins build is also successful but fail after the build in the publish bit, which seems unrelated. 05:45:15 [INFO] ------------------------------------------------------------------------ |
|
Thanks @ptziegler, looking forward for tomorrow I-build improved target platform resolution |
From the documentation:
The proper way is to replace calls to IProgressMonitor.beginTask(...) with SubMonitor.convert(...), keep the SubMonitor as a local variable and use that from now on.
This amends commit 9009085.