Bug 506447

Comment 1 Andrey Loskutov 2016-10-25 17:18:51 EDT

Just for the record, here is the stack trace from JUnit failure:

org.eclipse.core.runtime.OperationCanceledException
at org.eclipse.core.runtime.SubMonitor$RootInfo.checkForCancellation(SubMonitor.java:292)
at org.eclipse.core.runtime.SubMonitor.split(SubMonitor.java:995)
at org.eclipse.core.runtime.SubMonitor.split(SubMonitor.java:913)
at org.eclipse.core.tests.runtime.SubMonitorTest.testAutoCancelDoesNothingForTrivialConversions(SubMonitorTest.java:374)
at org.eclipse.test.EclipseTestRunner.run(EclipseTestRunner.java:754)
at org.eclipse.test.EclipseTestRunner.run(EclipseTestRunner.java:351)
at org.eclipse.test.CoreTestApplication.runTests(CoreTestApplication.java:37)
at org.eclipse.test.CoreTestApplication.run(CoreTestApplication.java:33)
at org.eclipse.equinox.internal.app.EclipseAppContainer.callMethodWithException(EclipseAppContainer.java:587)
at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:198)
at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:134)
at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104)
at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:388)
at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:243)
at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:673)
at org.eclipse.equinox.launcher.Main.basicRun(Main.java:610)
at org.eclipse.equinox.launcher.Main.run(Main.java:1519)
at org.eclipse.equinox.launcher.Main.main(Main.java:1492)
at org.eclipse.core.launcher.Main.main(Main.java:34)

Comment 2 Andrey Loskutov 2016-10-25 17:51:03 EDT

This is a regression from the commit 63f2c459b838fe7d9f9a44e00a0e1d6b98317f68 for bug 505936. Reverting the commit fixes the issue.

What wonders me, the test case seem to be wrong (or not testing what it should), because the javadoc of SubMonitor.split says:
"This method is much like newChild, but it will additionally check for cancellation and will throw an OperationCanceledException if the monitor has been cancelled."

Exact this happens now, but test code was added to verify that this should NOT happen:
public void testAutoCancelDoesNothingForTrivialConversions() {
	NullProgressMonitor npm = new NullProgressMonitor();
	npm.setCanceled(true);
	SubMonitor subMonitor = SubMonitor.convert(npm, 1000);
	subMonitor.split(1000); // <--- fails here now
}

So why should it not fail? If the assumption is still valid, we should at least better document the test case, because without additional info one would assume the test case is wrong.

Comment 3 Stefan Xenos 2016-10-26 00:41:46 EDT

No, the failure is real. Split isn't actually supposed to do a cancellation check on every invocation. One of the circumstances in which it is supposed to omit the cancellation check is when the call to split is itself a no-op which didn't actually split any ticks into the new monitor.

I'll fix it, and clarify the JavaDoc if this isn't clear.

Comment 4 Stefan Xenos 2016-10-26 02:26:07 EDT

Re: comment 2

I actually re-read the documentation for SubMontor.split and I think both the JavaDoc and the test are correct. The sentence following the one you quoted clarifies the contract. In context:

"This method is much like {@link #newChild}, but it will additionally check for cancellation and will throw an OperationCanceledException if the monitor has been cancelled. Not every call to this method will trigger a cancellation check. The checks will be performed as often as possible without degrading the performance of the caller."

The test is there to verify that no cancellation checks are performed for these no-op conversions (which almost always occur immediately after another operation what would have performed a cancellation check already).

Comment 5 Eclipse Genie 2016-10-26 04:19:50 EDT

New Gerrit change created: https://git.eclipse.org/r/83912

Comment 6 Eclipse Genie 2016-10-26 16:29:03 EDT

Gerrit change https://git.eclipse.org/r/83912 was merged to [master].
Commit: http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=f470ac39f9850055bae81b4d8b3bd59ecf0b674e