Bugzilla – Full Text Bug Listing
| Summary: | [jobs] Deadlock with ILocks possible due to improper cleanup of ILocks not released when Job ends | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Min Idzelis <min123> | ||||||
| Component: | Runtime | Assignee: | John Arthorne <john.arthorne> | ||||||
| Status: | CLOSED WONTFIX | QA Contact: | |||||||
| Severity: | major | ||||||||
| Priority: | P3 | CC: | bsd, sptaszkiewicz, thatnitind | ||||||
| Version: | 3.4.2 | ||||||||
| Target Milestone: | --- | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows XP | ||||||||
| Whiteboard: | stalebug | ||||||||
| Attachments: |
|
||||||||
I think we can probably help in this case, but this is still a programming error in your code that you failed to release the lock, so this would be an enhancement. Actually, this can be even worse. If you have a ILock being acquired in a job, and not released, that Job is sent back to the pool. If you then try to acquire that same lock in a different Job (that happens to be running on a recycled thread that was run by the first Job) the ILock.acquire() will proceed, since it uses it's currentOwnerThread field to see if thread is the owner. This breaks the semantics of ILock#acquire. The ILock class spec states that ILock.release() calls should be done in a finally block, so it is still a bug in client code that they failed to release the lock. This change actually has the potential to mask serious bugs in client code. I'm in total agreement with your assessment that this is improper coding by the client. However, the framework does "fix" the improper coding for the IJobManager.beginRule() case (which is also javadoc'ed to say that endRule() should be called in a finally block on the same thread). It also unconditionally logs message saying that there is improper coding: Status ERROR: org.eclipse.core.jobs code=1 Worker thread ended job: Test with rule(0), but still holds rule: ThreadJob(Test with rule(0),[Test$1@4b5c4b5c,]) null I just think that the framework should handle ILock the same way. I think these fixes are "ok" (as long as there is unconditional logging) because from what I gather, the intent of the Jobs/Locks/Scheduling rules framework was to make it easy to write deadlock resistant code. One could claim that Circular ILock acquires (which would normally deadlock an ordinary lock) is incorrect client programming. Yet, ILocks can automatically re-order themselves to prevent this type of deadlock. I think that ILocks should also automatically be released in cases where this situation can be detected. (When acquired during the execution of a Job.) Just a ping... What do you think the probability of getting this into Eclipse 3.5 is? Created attachment 227414 [details]
Patch v.0.1
The patch shows how we can release unreleased locks just before returning worker thread back to the pool. New test case added, all tests pass.
Converted to Gerrit change: https://git.eclipse.org/r/#/c/30197/ I wonder if releasing a lock is the same as ending a rule. It could well be that the resources guarded by the lock aren't in a good state (e.g., an uncaught exception occurred). An error message that the lock hadn't been released might be all that can be safely assumed? 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. |
Created attachment 126039 [details] Demo of deadlock Build ID: Eclipse 3.4.x Steps To Reproduce: The javadocs say that ILocks are capable of recovering from deadlocks caused amongst ILocks. I found a way to create a deadlock using only a single ILock. ILocks are unlike ISchedulingRules, where if there was an "beginRule" inside of a job, and then that Job ends, the ISchedulingRule will automatically be ended. For ILocks, the lock is not released if the Job that acquired the ILock ends without releasing it explicitely. This is a little inconsistent. Since ILocks and the JobManager are intimately tied together, I think that the JobManager should release ILocks when Jobs end to try to prevent deadlock, like it does for scheduling rules. See attached test case.