Bugzilla – Full Text Bug Listing
| Summary: | [jobs] core.jobs should not use #equals to compare ISchedulingRules | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Szymon Ptaszkiewicz <sptaszkiewicz> | ||||||||||||
| Component: | Runtime | Assignee: | platform-runtime-inbox <platform-runtime-inbox> | ||||||||||||
| Status: | CLOSED WONTFIX | QA Contact: | |||||||||||||
| Severity: | normal | ||||||||||||||
| Priority: | P3 | CC: | john.arthorne, remy.suen | ||||||||||||
| Version: | 3.7 | Flags: | sptaszkiewicz:
review?
(john.arthorne) |
||||||||||||
| Target Milestone: | --- | ||||||||||||||
| Hardware: | All | ||||||||||||||
| OS: | All | ||||||||||||||
| Whiteboard: | stalebug | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Szymon Ptaszkiewicz
Created attachment 206413 [details]
Patch v1
Work in progress. Tests and javadoc for the new class should be added.
Created attachment 217111 [details]
Patch v2
Javadoc and tests added. All runtime and resources tests pass.
Creating a custom data structure seems complicated. How about using a wrapper object rather than putting the job directly in the array? I thought about wrapper object earlier when preparing Patch v1 and I wrote even initial patch with this solution, but it soon turned out that this would be more like circumventing the problem than fixing it the right way. Custom data structure may seem to be a complex change but it has several advantages when comparing to wrapper object: - wrapper object solution requires updating all places where we use "add" and "get" so that proper boxing-unboxing can happen. There are many "get"s used so changing it just for proper unboxing would be redundant and would make implementation unnecessary complicated, especially in DeadlockDetector class - wrapper objects cannot be casted to ILock - using a dedicated class changes the existing code only in places where it is absolutely necessary - if we used wrapper object, it would need to be of ISchedulingRule type. We wouldn't know if a rule in array was already wrapped or not. - we don't need to cover the whole API of java.util.List so array implementation can be simpler - there is no additional memory overhead related to creating new wrapper object for each rule - since implementation is simpler than default, overall performance and memory consumption can also be expected to be better - type verification is a nice bonus, no need for casts - we add additional data structure that needs to be maintained but this is not that severe because we already did it in the past (see Queue or many examples of dedicated data structures in o.e.jdt.core.internal.compiler.util package) There is also one point that is more like side-effect of choosing custom data structure but gives us additional chance to increase proper API usage verification: - it is hard to find a place in core.jobs where we could have two rules available on which we could perform sanity checks against ISchedulingRule API. SchedulingRuleList#areEqual seems like a perfect place for that (I see now that Patch v2 could be even improved to increase verification even more). Created attachment 218866 [details] Patch v3 Verification improved as mentioned in comment 4. John, can you please review? (In reply to comment #4) > There is also one point that is more like side-effect of choosing custom > data structure but gives us additional chance to increase proper API usage > verification: > - it is hard to find a place in core.jobs where we could have two rules > available on which we could perform sanity checks against ISchedulingRule > API. SchedulingRuleList#areEqual seems like a perfect place for that (I see > now that Patch v2 could be even improved to increase verification even more). This point no longer applies. Bug 393748 shows that implementations of ISchedulingRule may violate API in a completely unexpected way so it doesn't make sense to add additional verification only in one place. If we decided to add it in the future, we should add it everywhere and that should be handled in a separate bug. I will remove the verification code from the patch, so that the patch contains only relevant changes. Created attachment 227594 [details]
Patch v4
The patch is the same as Patch v1 with added javadoc and tests.
Created attachment 231314 [details]
Regression test
Finally, I found some time to prepare a test showing the problem using API methods only. The test shows valid ISchedulingRule implementation where starting two threads with two separate rules causes unexpected state of deadlock detector. Deadlock detector should be aware of two rules but since it uses the equals method it thinks that one rule was acquired by two different threads which is bogus. Correct behaviour of this test can be obtained after applying Patch v4.
Converted to Gerrit change: https://git.eclipse.org/r/#/c/30198/ This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie. |