Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 361302

Summary: AbstractStructuredModel deadlock when reusing lock from JobSafeStructuredDocument
Product: [WebTools] WTP Source Editing Reporter: Nick Sandonato <nsand.dev>
Component: wst.sseAssignee: Nick Sandonato <nsand.dev>
Status: RESOLVED FIXED QA Contact: Nitin Dahyabhai <thatnitind>
Severity: normal    
Priority: P3 CC: cbridgha, neil.hauge
Version: 3.2.5Flags: nsand.dev: pmc_approved? (david_williams)
nsand.dev: pmc_approved? (raghunathan.srinivasan)
nsand.dev: pmc_approved? (naci.dai)
nsand.dev: pmc_approved? (deboer)
neil.hauge: pmc_approved+
nsand.dev: pmc_approved? (kaloyan)
cbridgha: pmc_approved+
thatnitind: review+
Target Milestone: 3.2.5   
Hardware: PC   
OS: Mac OS X - Carbon (unsup.)   
Whiteboard: PMC_approved, WI80847
Attachments:
Description Flags
patch none

Description Nick Sandonato CLA 2011-10-18 15:50:26 EDT 
A lock is shard between the JobSafeStructuredDocument and the AbstractStruturedModel when aboutToChangeModel/modelChanged pair are invoked. This lock is largely ineffective, though, as only one thread ever truly attempts to require it as the model is changing, so other threads will proceed as normal. However, it has a potential to cause a deadlock in the case where modelChanged is not invoked after aboutToChangeModel.

To avoid this problem, I'm proposing that this lock is no longer acquired from the model. The internal state will continue to be used to correctly fire the appropriate events, however. All changes to the structured document are still controlled by the lock as before.
Comment 1 Nick Sandonato CLA 2011-10-18 15:53:10 EDT 
Created attachment 205455 [details]
patch
Comment 2 Nick Sandonato CLA 2011-10-19 14:09:55 EDT 
* Explain why you believe this is a stop-ship defect. Or, if it is a "hotbug" (requested by an adopter) please document it as such.

A deadlock is caused by this shared lock. This also prevents any future changes by any other thread to the structured document holding this lock.

* Is there a work-around? If so, why do you believe the work-around is insufficient?

Yes, appropriately pairing aboutToChangeModel/changedModel should fix this; however, it is impossible for us to police all of these cases. All it takes is one thread to fail to pair it properly, and things can go downhill.

* How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added?

All existing unit tests have been run. Adhoc testing has been executed.

* Give a brief technical overview. Who has reviewed this fix?

The shared ILock will no longer be used by the AbstractStructuredModel. This will prevent unbalanced calls to aboutToChangeModel/changedModel from causing deadlocks. The events fired from aboutToChangeModel/changedModel still act the same way. All changes to the document are still appropriately guarded by this lock.  Nitin has reviewed the fix.

* What is the risk associated with this fix?
Fairly low. This lock has never really been used to guard any real changes to the model, as far as I can tell. One can see this in normal model changes where this lock is never needed to be acquired.
Comment 3 Chuck Bridgham CLA 2011-10-20 10:02:21 EDT 
We ran the Java EE junit bucket as well on this patch to confirm the safety of this fix.   No change, so I will approve
Comment 4 Neil Hauge CLA 2011-10-20 14:14:34 EDT 
From discussion it appears this deadlock has been seen in the wild and this will solve these and possible future deadlocks.  +1
Comment 5 Nick Sandonato CLA 2011-10-20 14:54:50 EDT 
Code released to 3_2_Maintenance.
Comment 6 Nick Sandonato CLA 2011-10-20 14:56:17 EDT 
Tracking 3_3_Maintenance and HEAD through Bug 361583.