Bugzilla – Full Text Bug Listing
| Summary: | MultiRule#equals is missing | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Szymon Ptaszkiewicz <sptaszkiewicz> | ||||
| Component: | Runtime | Assignee: | platform-runtime-inbox <platform-runtime-inbox> | ||||
| Status: | RESOLVED INVALID | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | john.arthorne, remy.suen | ||||
| Version: | 3.7 | ||||||
| Target Milestone: | --- | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
I will try to prepare a patch for that. Hmm, I think it is dangerous to rely on the implementation of equals for an ISchedulingRule. Since the client can implement this interface, we can't assume that the equals method on all objects implementing ISchedulingRule has any relationship to the contains method. For example Resource#contains only looks at the path, whereas Resource#equals looks at the path, workspace, and type. Maybe the javadoc on ISchedulingRule#contains should be reworded here. Exactly. And that means that whenever we need to check if two ISchedulingRules are equal, we should use a.contains(b) && b.contains(a) instead of a.equals(b). Since this change is going to be a bigger code change in core.jobs, I will open another bug for that and in this bug we just handle problems with MultiRule. Regarding the Resource#contains method, I think we should add a check for workspace there. After that, when we eventually implement multi-workspace support, two resources with the same path but in different workspace would not be in conflict with each other. That of course requires a separate bug in Platform/Resources. Opened bug 362512 and bug 362515. (In reply to comment #2) > Hmm, I think it is dangerous to rely on the implementation of equals for an > ISchedulingRule. Since the client can implement this interface, we can't assume > that the equals method on all objects implementing ISchedulingRule has any > relationship to the contains method. For example Resource#contains only looks > at the path, whereas Resource#equals looks at the path, workspace, and type. > Maybe the javadoc on ISchedulingRule#contains should be reworded here. For any two scheduling rules a and b, if a.equals(b), then a.contains(b) && b.contains(a). But, if a.contains(b) && b.contains(a), then it does not necessarily imply that a.equals(b) because a and b may be of different type. So, I guess there are two separate definitions of equality: 1) Scheduling rules equality: two rules are equal iff a.contains(b) && b.contains(a). 2) Objects equality: two objects are equal iff a.equals(b). We now get two implications: A) 1) implies 2) B) 2) implies 1) B) is always correct for any two scheduling rules, but A) is true only for scheduling rules of the same type. Because of that, I think that the javadoc of the contains method is wrong. Antisymmetric property should be valid for the whole set of all scheduling rules not only for the particular implementation (for one type). Especially, we should avoid the "equals" there. ISchedulingRule#contains deals with members of the relation whereas Object#equals deals with objects and I think we should not mix those two together. Created attachment 214447 [details]
Patch v1
Your patch doesn't seem to match the analysis. I don't think we can assume anything about the equals method on implementations of ISchedulingRule. So we should really just avoid talking about equals rather than adding implementations of equuals to our rules. (In reply to comment #7) > Your patch doesn't seem to match the analysis. I don't think we can assume > anything about the equals method on implementations of ISchedulingRule. So we > should really just avoid talking about equals rather than adding > implementations of equuals to our rules. In that case, I think this bug is INVALID and we should get rid of all references to ISchedulingRule#equals (bug 362512). Does it sound reasonable? Yes, thanks Szymon. |
Current implementation of MultiRule violates ISchedulingRule contract because of missing #equals method. Consider the following example: MultiRule multi1 = new MultiRule(new ISchedulingRule[] {child1, child2}); MultiRule multi2 = new MultiRule(new ISchedulingRule[] {child2, child1}); multi1.contains(multi2)); => true multi2.contains(multi1)); => true multi1.equals(multi2)); => false but should be true Without proper implementation of MultiRule#equals we are not able to satisfy the requirement for antisymmetric MultiRule#contains. Missing #equals is also dangerous in cases where #equals is used implicitly (lists, sets, etc.). See for example DeadlockDetector#locks and DeadlockDetector#indexOf(ISchedulingRule, boolean).