ProfileChangeOperation: fix progressmonitor usage #335 #336

jukzi · 2023-09-26T16:04:12Z

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

#335

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

github-actions · 2023-09-26T16:25:10Z

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 · 2023-10-08T13:26:00Z

...act.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/RawMirrorRequest.java

-			return;
-		}
-		IStatus status = transfer(targetDescriptor, sourceDescriptor, monitor);
+		SubMonitor subMon = SubMonitor.convert(monitor,NLS.bind(Messages.downloading, getArtifactKey().getId()),1);


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.

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.

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 ...

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.

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:

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.

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

HannesWell · 2023-10-08T13:30:00Z

...act.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/RawMirrorRequest.java

-			if (steps.isEmpty()) {
-				LogHelper.log(new Status(IStatus.WARNING, Activator.ID,
-						NLS.bind(Messages.noDigestAlgorithmToVerifyDownload, artifactDescriptor.getArtifactKey())));
+		monitor = IProgressMonitor.nullSafe(monitor);


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.

all my concerns are expressed by the junit result.

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 · 2023-10-25T11:58:33Z

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

HannesWell · 2023-10-25T12:46:56Z

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

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

HannesWell · 2023-10-29T14:32:48Z

@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 · 2023-11-02T07:23:10Z

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

@HannesWell the tests fail now - which shows the problem

HannesWell · 2023-11-02T22:24:40Z

@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 · 2023-11-03T07:28:04Z

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.

passing a monitor to two callees requires it to be splitted. eclipse-equinox#335 Also fixed forgotten monitor.done() in performProvisioningPlan and improved tests.

HannesWell · 2023-11-05T11:39:09Z

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.

jukzi force-pushed the monitor_done()_ branch 2 times, most recently from 07048c5 to a13a0eb Compare September 28, 2023 12:35

HannesWell force-pushed the monitor_done()_ branch from a13a0eb to a1592a0 Compare October 8, 2023 12:48

HannesWell reviewed Oct 8, 2023

View reviewed changes

HannesWell force-pushed the monitor_done()_ branch 2 times, most recently from 4962ea4 to d97e3cd Compare October 29, 2023 14:28

HannesWell force-pushed the monitor_done()_ branch from d97e3cd to 25832ef Compare October 29, 2023 14:33

HannesWell force-pushed the monitor_done()_ branch 2 times, most recently from 25832ef to 58d2455 Compare November 5, 2023 09:33

EcljpseB0T added 2 commits November 5, 2023 11:31

version bump org.eclipse.equinox.p2.tests.ui

99fde52

Fix IProgressMonitor usage eclipse-equinox#335

a24950d

passing a monitor to two callees requires it to be splitted. eclipse-equinox#335 Also fixed forgotten monitor.done() in performProvisioningPlan and improved tests.

HannesWell force-pushed the monitor_done()_ branch from 72e668f to a24950d Compare November 5, 2023 10:31

HannesWell merged commit e31c371 into eclipse-equinox:master Nov 5, 2023
8 of 9 checks passed

jukzi deleted the monitor_done()_ branch November 6, 2023 07:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ProfileChangeOperation: fix progressmonitor usage #335 #336

ProfileChangeOperation: fix progressmonitor usage #335 #336

jukzi commented Sep 26, 2023

github-actions bot commented Sep 26, 2023 •

edited

Loading

HannesWell Oct 8, 2023

laeubi Oct 8, 2023

laeubi Oct 8, 2023

laeubi Oct 8, 2023

jukzi Oct 16, 2023

laeubi Oct 16, 2023 •

edited

Loading

HannesWell Oct 8, 2023

jukzi Oct 29, 2023

HannesWell Oct 29, 2023

HannesWell left a comment

jukzi commented Oct 25, 2023

HannesWell commented Oct 25, 2023

HannesWell commented Oct 29, 2023 •

edited

Loading

jukzi commented Nov 2, 2023

HannesWell commented Nov 2, 2023

laeubi commented Nov 3, 2023

HannesWell commented Nov 5, 2023

ProfileChangeOperation: fix progressmonitor usage #335 #336

ProfileChangeOperation: fix progressmonitor usage #335 #336

Conversation

jukzi commented Sep 26, 2023

github-actions bot commented Sep 26, 2023 • edited Loading

Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laeubi Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HannesWell left a comment

Choose a reason for hiding this comment

jukzi commented Oct 25, 2023

HannesWell commented Oct 25, 2023

HannesWell commented Oct 29, 2023 • edited Loading

jukzi commented Nov 2, 2023

HannesWell commented Nov 2, 2023

laeubi commented Nov 3, 2023

HannesWell commented Nov 5, 2023

github-actions bot commented Sep 26, 2023 •

edited

Loading

laeubi Oct 16, 2023 •

edited

Loading

HannesWell commented Oct 29, 2023 •

edited

Loading