Bugzilla – Full Text Bug Listing
| Summary: | [Progress] DetailedProgressViewer's comparator violates its general contract | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Dani Megert <daniel_megert> |
| Component: | UI | Assignee: | Paul Webster <pwebster> |
| Status: | VERIFIED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | ericdp, javadev, loskutov, nitram509, pwebster, remy.suen, simon.goodall |
| Version: | 3.8 | ||
| Target Milestone: | 4.2 M5 | ||
| Hardware: | PC | ||
| OS: | Windows 7 | ||
| See Also: |
https://bugs.eclipse.org/bugs/show_bug.cgi?id=525287 https://bugs.eclipse.org/bugs/show_bug.cgi?id=562314 |
||
| Whiteboard: | |||
| Attachments: | |||
|
Description
Dani Megert
The bug is in org.eclipse.ui.internal.progress.JobTreeElement.compareTo(Object) or one of its subclasses. My guess would be: org.eclipse.ui.internal.progress.JobInfo with it's convoluted compareTo. PW (In reply to comment #2) > My guess would be: org.eclipse.ui.internal.progress.JobInfo with it's > convoluted compareTo. Likely. There seems to be a wide variety of issues here. Some or all of which may be contributing to this error. JobTreeElement: -implements compareTo(Object) but returns 0 if it's not a JobTreeElement, sounds like a CCE is better? -no equals(Object) implementation ErrorInfo: -reimplements compareTo(Object) but doesn't implement equals(Object) GroupInfo: -(unnecessarily) reimplements compareTo(Object) but doesn't implement equals(Object), its code is almost identical to JobTreeElement JobInfo: -same as ErrorInfo -consider the case where the two objects are equal except their states and neither of their states are Job.RUNNING, then _both_ jobInfoA.compareTo(jobInfoB) _and_ jobInfoB.compareTo(jobInfoA) will return 1 (at the end of the method) See bug 364735 comment #44 for an example of our logging when this fails. PW It was org.eclipse.ui.internal.progress.JobInfo. P.S. Please consider to backport the fix to 3.7.2 too (current target is 3.8). (In reply to comment #5) > It was org.eclipse.ui.internal.progress.JobInfo. > P.S. Please consider to backport the fix to 3.7.2 too (current target is 3.8). What do you mean by that "... it was ...JobInfo" ? Is there a patch/commit link available? Regards Martin (In reply to comment #6) > What do you mean by that "... it was ...JobInfo" ? > Is there a patch/commit link available? He means of the possible types that could cause this bug listed in comment #3 JobInfo seems to be the culprit. This is not the same root cause as the other comparator bug. PW (In reply to comment #7) > (In reply to comment #6) > > What do you mean by that "... it was ...JobInfo" ? > > Is there a patch/commit link available? > > He means of the possible types that could cause this bug listed in comment #3 > JobInfo seems to be the culprit. > > This is not the same root cause as the other comparator bug. > > PW Understood. Looking at JobInfo.java I also would vote for it, being the culprit. Regards Martin Created attachment 208414 [details]
JUnit (3.8) test case, proving that JobInfo violates compareTo() method contract
Iv've created a JUnit Test to prove, that JobInfo violates compareTo()
method contract (see attachment).
Please, can someone verify?
After verification I would like to continue code a new compareTo() implementation.
Created attachment 208452 [details] JobInfo compareTo test Hi Martin, I've turned your file into a patch that applies against master. I haven't quite looked at it yet, but could you use http://wiki.eclipse.org/Platform_UI/How_to_Contribute and this patch to contribute any more changes you'd like to see us look at. Thanx for the tests, PW (In reply to comment #10) > Created attachment 208452 [details] > JobInfo compareTo test > > Hi Martin, > > I've turned your file into a patch that applies against master. I haven't > quite looked at it yet, but could you use > http://wiki.eclipse.org/Platform_UI/How_to_Contribute and this patch to > contribute any more changes you'd like to see us look at. > > Thanx for the tests, > PW Thanks. I'm curious, what you will report, after running the test. What exactly my patched was missing in contrast to the Wiki how to (where can I improve) ? (In reply to comment #11) > What exactly my patched was missing in contrast to the Wiki how to > (where can I improve) ? You attached a raw Java file and not a patch (a diff) that can be applied by Eclipse. (In reply to comment #11) > I'm curious, what you will report, after running the test. The comment I have on the test itself is that it's just a pass or fail. Is there no way to break it up to several tests so that fixing a failure can show progress? if not, don't worry about it, I think the test is good. > > What exactly my patched was missing in contrast to the Wiki how to > (where can I improve) ? I appreciate you providing a test ... tests are good :-) JobInfoTest needed to go in our test plugin, org.eclipse.ui.tests, and required a minor mod to JobInfo to compile. As Remy mentioned, we take contributions are either patches (the format that needs to be attached to a bug) or pull requests. I thought I'd just provide the starting point, pointing you to wiki docs is OK but an apply-able patch is better :-) PW Created attachment 208524 [details] JUnit (3.8) test case, proving that JobInfo violates compareTo() method contract Reworked the test to be a patch. Also added my name and email within contribution section. So I think, Paul Websters attachment made in Comment #10 should be obsolete. Created attachment 208525 [details]
JUnit (3.8) test case, checks when jobs sorted by their state, the running ones are ordered to first place
Adding one more Test.
This will work fine with current implementation but is an requirement
for upcoming FIX.
Created attachment 208526 [details]
FIX that correctly compares job states, according to method contract
Hi,
thanks to the unit test, the root cause was easy to find:
When comparing job states, all states (none, sleeping, waiting, running)
have to be compared by their correlated integer value.
If not (before this FIX), the comparator method contract was violated.
The FIX looks similar to Integer.compareTo() but inverses the ordering,
because we want to have running jobs in pool position.
From my point of view, this bug can be reviewed and closed now ;-)
Regards
Martin
Created attachment 208705 [details] Patch that describes all 3 changes This is the combined patch for 3 attachments from Martin. Martin, if you look at stuff in the future can you use http://wiki.eclipse.org/Platform_UI/How_to_Contribute ? We're in the transition to git and the patches you gave tend to be CVS patches. I've made sure they apply clean. PW Released to 4.2 http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=80605f139b12c3f0afae6a7b8d2a25bc713430ff Released to 3.8 http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=827da075b5564198c2399b94eb0bce7d1375e3e9 Thanx Martin, PW The changes are in I20120123-2200 PW Is this bug specific to eclipse running on Java 7? This forum post http://www.eclipse.org/forums/index.php/m/756308/ suggests that JRE 1.7 no longer tolerates this exception as it did previously. I'm interested to get a list of bugs related to eclipse running on Java 7 that exist post eclipse 362. (In reply to comment #20) > Is this bug specific to eclipse running on Java 7? Yes. > I'm interested to get a > list of bugs related to eclipse running on Java 7 that exist post eclipse 362. There can be many related to wrongly implemented comparators. The most severe one being bug 317785. Instead of trying to catch all these, you'd better set the following system property in your product: -Djava.util.Arrays.useLegacyMergeSort=true *** Bug 385485 has been marked as a duplicate of this bug. *** *** Bug 403533 has been marked as a duplicate of this bug. *** |