ProfileChangeOperation: fix progressmonitor usage #335

[jukzi]

passing a monitor to two callees requires it to be splitted.

#335

Also fixed forgotten monitor.done() in performProvisioningPlan and improved tests.

[github-actions]

Test Results

       9 files         9 suites   33m 50s ⏱️
2 181 tests 2 177 ✔️   4 💤 0 ❌
6 633 runs  6 622 ✔️ 11 💤 0 ❌

Results for commit a24950d.

♻️ This comment has been updated with latest results.

[HannesWell]

Converting the monitor into a SubMonitor with work one does not look necessary here and it can just be passed directly.
Furthermore I think it is not necessary to add the try-catch-block to ensure done is called in any ways.
Just calling done below is sufficient and if the method throws the method returns any way and a caller that catches an exception either calls done on the passed monitor or (should) call split which also cleans up children.

[laeubi]

Furthermore I think it is not necessary to add the try-catch-block to ensure done is called in any ways

Correct, IProgressmonitor states:

CALLER/CALLEE RESPONSIBILITIES:

Methods that receive an IProgressMonitor ("callees") must either obey the following conventions or include JavaDoc explaining how it differs from these rules. The called method:

Will call beginTask on the argument 0 or 1 times, at its option.
Will not promise to invoke done() on the monitor.
Will not call setCanceled on the monitor.
May rely on the monitor not being null.
May rely on the monitor ignoring the string passed to beginTask.

[laeubi]

Furthermore I think it is not necessary to add the try-catch-block to ensure done is called in any ways

Correct, IProgressmonitor states:

CALLER/CALLEE RESPONSIBILITIES:

Methods that receive an IProgressMonitor ("callees") must either obey the following conventions or include JavaDoc explaining how it differs from these rules. The called method:

Will call beginTask on the argument 0 or 1 times, at its option.
Will not promise to invoke done() on the monitor.
Will not call setCanceled on the monitor.
May rely on the monitor not being null.
May rely on the monitor ignoring the string passed to beginTask.

[laeubi]

furthermore I think it is not necessary to add the try-catch-block to ensure done is called in any ways.

Yep, see javadoc of IProgressMonitor ...

[jukzi]

By passing a submonitor with progress one and unconditional calling done() the progress is unconditional increased which ensures to use up all the reserved progress. That is technically not necessary but ensures a user readable progress from 0 to 100% . This commit fixes the error and proofs that with junit test. This pattern is easy to understand and not error prone. I am not interested in further "optimizations" that "may" work under special conditions.

[laeubi]

I just wanted to note that when one splits a monitor previous aquired "splits" will be completed automatically, so if the toplevel is calling done() and the other code is always using split() progress si still reported correctly in every case.

This is to ease fulfill the contract responsibility of IProgressMonitor for the called method:

1. Will call beginTask on the argument 0 or 1 times, at its option.
2. Will not promise to invoke done() on the monitor.
3. Will not call setCanceled on the monitor.
4. May rely on the monitor not being null.
5. May rely on the monitor ignoring the string passed to beginTask.

so calling done is of course allowed, but not required by a method receiving a monitor.

[HannesWell]

SubMonitor.convert can handle null monitors and can't we just call done on the sub-monitor?
Again I think the try-catch is not necessary.

[jukzi]

all my concerns are expressed by the junit result.

[HannesWell]

But asserting in AbstractProvisioningUITest that for example ProvisioningSession.performProvisioningPlan() consumes all work of the monitor is not right.

From the Java-doc of IProgressMonitor:
https://github.com/eclipse-equinox/equinox/blob/8beb9ce0b1dea1176b06ad7300f2663ff6db4eff/bundles/org.eclipse.equinox.common/src/org/eclipse/core/runtime/IProgressMonitor.java#L57

CALLER/CALLEE RESPONSIBILITIES: 

Methods that receive an {@link IProgressMonitor} ("callees") must either obey the following conventions or include  these rules. The called method:
[...]
- Will not promise to invoke {@link #done()} on the monitor.

[...]
The caller:
[...]
Will not rely on the callee to invoke {@link #done()} on the monitor. It must either select an implementation of {@link IProgressMonitor} that does not require {@link #done()} to be called, for example {@link SubMonitor}, or  it must invoke {@link #done()} itself after the method returns.

And the only real caller of ProvisioningSession.performProvisioningPlan() in ProfileModificationJob.runModal() even explicitly calls done, although even that would actually not be necessary since the caller of that method respectively the creator of a progress-monitor is responsible to call done.

So asserting that in the tests is too strict.

[HannesWell]

Thanks Jörg for this PR.
In general in most places SubMonitor.convert() can be used directly and calling IProgressMonitor.nullSafe() seems not necessary.

Furthermore as already written in the detailed comments, a try-catch-block is often not necessary because in case of an exception the method terminates immediatly and progress-reporting usually stops anyways.

Besides the already covered locations it looks like MirrorRequest.transfer() also seem not to handle its monitor correctly, just like SimpleArtifactRepository.getArtifact() and getRawArtifact().

[jukzi]

Mind to submit as-is if nobody works on further improvements?

[HannesWell]

Mind to submit as-is if nobody works on further improvements?

I can take care of this later/in the next days.

[HannesWell]

@jukzi as discussed I now corrected/simplified the usage of the SubMonitor.
Please have a look if your main points are still addressed.

In some places I used newChild() instead of split() since split throws OperationCancledException, which is actually more convenient, but the calling code seems to expect a canceled status instead.

[jukzi]

@jukzi as discussed I now corrected/simplified the usage of the SubMonitor.

@HannesWell the tests fail now - which shows the problem

[HannesWell]

@jukzi as discussed I now corrected/simplified the usage of the SubMonitor.

@HannesWell the tests fail now - which shows the problem

As said above it is not a problem. A callee does need not call done on the received monitor as the doc states.
It is perfectly fine if a method has e.g. ten pieces of work to do if it returns and the monitor only has 9/10 pieces of work consumed.
It is the responsibility of the creator of of a Monitor to call done on it.
Therefore I think the new assumption in the tests is not right.

Did you encounter any creator of a Monitor that did not call done? If yes that should be fixed instead of requiring the calles to call done. Of course they should consume some amounts of work, but the last bit can be left out. Therefore the SubMonitor.split/newChild() pattern was introduced, which actually makes it relatively simple to use monitors right.

[laeubi]

Of course they should consume some amounts of work

A receiver of the monitor might or might not call any method on the monitor... The good thing about submonitor is that one can reuse unused progress if desired.

[HannesWell]

I know changed the two methods that the test complains about (although I still have the opinion that they are too strict) to call done at the end of the method, but not in a finally block. This way all the callers that make false assumptions should be happy too.

This way we can have at least the more important changes to avoid multiple monitor usages.