This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
Summary: | Deadlock when PositionRef is started to be added prior to document opening | ||
---|---|---|---|
Product: | platform | Reporter: | Miloslav Metelka <mmetelka> |
Component: | Text | Assignee: | Petr Nejedly <pnejedly> |
Status: | VERIFIED FIXED | ||
Severity: | blocker | CC: | jskrivanek, jtulach, mmirilovic, ttran |
Priority: | P2 | Keywords: | THREAD |
Version: | 3.x | ||
Hardware: | All | ||
OS: | All | ||
Issue Type: | DEFECT | Exception Reporter: | |
Bug Depends on: | 40951 | ||
Bug Blocks: | |||
Attachments: |
Apologies, same threads as text attachment
Proposed patch for release36 branch Test failing test Fix to make both tests succeed |
Description
Miloslav Metelka
2004-03-04 16:05:22 UTC
Created attachment 13829 [details]
Apologies, same threads as text attachment
The deadlock happens like this: Thread 1) a) PositionRef$Manager.addPosition() gets called. b) PositionRef$Manager.doc is tried to be read-locked but it is null so the locking is skipped c) monitor on CloneableOpenSupport$Listener is entered d) PositionRef$Manager$Kind.toMemory() gets called but this time the PositionRef$Manager.doc is already assigned so it's attempted to be read-locked by DocumentRenderer but it waits for the other thread that does NbDocument.runAtomic() which already acquired write-lock on it. Thread 2) a) NbDocument.runAtomic() write-locks the document. b) An attempt to openDocument() will cause the monitor on CloneableOpenSupport$Listener to be entered which counter-locks with the first thread. Petre, not sure whether this is doable but IMHO we could try to avoid the situation when the second nested doc-renderer sees the loaded document. It should get null as well so that the counter-locking does not occur. We can give it null using ThreadLocal magic. I'll look at it more, it may be possible to solve it better way. Idea: PR$Manager already wraps all accesses to document into DocumentRenderer, regardless of current availability of Document. What if we align all these document accesses with Mutex, so use the Mutex (always serialized the same way as document) as a guard in case of null document. We can also use its writeRequest for actually setting the document instance, thus prevent document from popping up in teh middle of to/fromMemory conversion. Seems it may work. I'm running it currently. Needs more testing. My current solution acquires mutex readlock for all read-document accesses from PositionRef.Manager and acquires mutex write lock for all document setting calls. This means that one can acquire a readlock on the document and then call a method on PR.M, which then tries to acquire read lock on the mutex -> different lock order. But these are read locks, which can be acquired cuncurrently, os it should be no problem. If I change the order (read-lock the mutex from inside the document's read access with carefull rollback-retry for null-document-before-real-document-now), lhe lock order will be OK, but the code will be more compilcated. Mila's idea of simulating null document during whole call looked promising, but might cause bad incosistencies as some threads would operate on the manager structures thinking there is no doc, while other will see a doc. My fix was integrated into trunk, openide/src/org/openide/text/PositionRef.java,v1.52 Petr, Mila, do you plan/want to fix this for NB3.6 ? If so - ask for review, else - ask for waiver...... No. I've rolled back the change and have to reopen this, as my fix caused another deadlock. Another idea: Always keep (at least dummy) document in PR.M for the purpose of locking. Just a raw idea: I'm wondering whether we could call getDocument() prior to the actual creation of the PositionRef. If the document would not be opened that would be no-op returning null. If it would just be in the process of being loaded by RP thread then the particular thread would become blocked until the loading finishes and the document becomes known. Of course we need to prevent deadlocking of the RP thread itself when attaching positionref but AFAIK there should already be a threadlocal LOCAL_LOAD_TASK variable that should prevent that. Not sure though whether I did not overlook anything important. *** Issue 40856 has been marked as a duplicate of this issue. *** *** Issue 40865 has been marked as a duplicate of this issue. *** Fixed again, test added. openide/src/org/openide/text/CloneableEditorSupport.java,v1.118 openide/src/org/openide/text/PositionRef.java,v1.54 openide/test/unit/src/org/openide/text/Deadlock40766Test.java,v1.1 It appeared in continuos build of NB36: http://www.netbeans.org/download/release36/buildlogs/continuous/20040311-0730/xtest_results/testrun_040311-084050/logs/ide_qa-functional.log It could be fixed fixed in NB36, if the fix will be successful in trunk. Created attachment 13943 [details]
Proposed patch for release36 branch
I'd like to fix it in rel3.6. Yardo, could you please review the patch for release36 branch (the same as for dev, except it doesn't contain the test hooks and the test is ommited from the branch) We worked with Petr to create a test that would simulate the deadlock. We succeeded and reproduced the problem. Then Petr created his fix and the test started to pass. That is why it is easy to confirm that Petr's solution fixes the problem. However I have to say that the probability that another problem will be introduced is not zero. I think I have simulated a deadlock caused by this fix. I will attach the test. In my opinion the correct fix of both issues would use "enter barrier" for everyone who wants to change state of document and not whole sychronized block around the whole operations with document. Created attachment 13958 [details]
Test failing test
Created attachment 13966 [details]
Fix to make both tests succeed
remove 36_HR_FIX Integrated Yarda's test, integrated new test for 38013 deadlock: openide/test/unit/src/org/openide/text/Deadlock40766Test.java,v1.2 Fixed all 3 deadlocks using Yarda's enter barrier patch: openide/src/org/openide/text/CloneableEditorSupport.java,v1.120 openide/src/org/openide/text/PositionRef.java,v1.55 verified |