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.
There is several issues e.g. http://www.netbeans.org/issues/show_bug.cgi?id=33040 that seem to be caused by wrong or missing locking strategy at various places in the org.openide.text package. In this issue I would like to revisit the threading strategy of the text package and possibly suggest improvements that could be later incorporated into the openide's code.
Created attachment 10724 [details] Possible diff for PositionRef.java
Mila, sorry for delay in my answer. I forgot about this issue. If you think it will improve the state then go ahead and commit it. If there would be some problems later with the patch I would ask you to take care about them. From my point of view the issue must be addressed by some complete redesign of this package. That task is in the plan, but not scheduled and analyzed yet. TBD.
I am also interested in a possibly bigger redesign and simplification of threading in this package. The patch looks fine to me in the meantime.
I have completed o.o.t.DocumentLine too but I'm facing some deadlocks in certain situations e.g. with java's template wizard when trying to override methods from an existing superclass.
I'm blocked with an interesting problem. If I add e. g. sout of offset,line,column into PositionRef. Manager.OutKind constructor I get a threading issue. When I create a new class through a wizard and inherit from e.g. a javax.swing.JPanel and wish to override first e.g. 10 methods. In about 80% everything goes well and the empty bodies of the overrided methods are inserted into the text. In the remaining cases however the method bodies are generated either to the begining of the source or more often into the javadoc above the constructor. I don't know yet whether the problem lies in java module or in o.o.text but I'm trying to find that out intensively. I'm attaching the regular PositionRef (without any additional patching) just with the sout patch which causes the malfunction.
Created attachment 11197 [details] PositionRef.java with sout (and tds) that cause the problem when using wizard
Created attachment 11198 [details] Stack trace of OutKind creation. The document is being closed by JDO.updateSource()
Created attachment 11200 [details] Stacktrace of how the PositionRef.Manager.PositionKind gets created
Created attachment 11201 [details] How parser creates PositionKind.Manager.PositionKind through createStableBounds()
Created attachment 11204 [details] Attaching garbled class for completness - some methods are fine, some are in the javadoc of the constructor
Created attachment 11205 [details] Garbled class - all methods in javadoc section
What might be interesting too is that if I create a class without overriden methods and try to override them later then the problem does not occur.
Created attachment 11214 [details] Deadlock after some extra locking has been added to PositionRef showing how the template wizard can compete with the parser at the same time
Created attachment 11215 [details] Garbled C7.java - the "extends javax.swing.JComponent" makes the inserted text to go inside the javadoc
Created attachment 11216 [details] Debug of the thread that first inserts at the wrong place near to the end of the javadoc
Created attachment 11217 [details] Debug of the thread that first inserts at the wrong place near to the end of the javadoc
Created attachment 11218 [details] C7.java correction - now really garbled (the original one wasn't)
During searching for the cause of the garbling I've got an interesting deadlock which shows that the document can get changed in the PositionRef manager between two invocations of certain pieces of code that touch the document (and thus they are run under a Document.render() in the patched code). The first piece of code obtains the readlock and invokes the second piece of code that is also run under readlock. If there would be just one document the second readlock-ing would be noop as a readlock was already obtained on the document. However the second readlocking gets deadlocked (with a thread that already holds a write-lock). The only explanation I see is that there was a different document instance in the first readlock attempt than it is in the second attempt.
Created attachment 11231 [details] Deadlocked threads
Created attachment 11232 [details] Modified PositionRef.java with which the deadlock occurred
I think the issue is almost fully resolved now. I'm attaching an html document that I've created during fixing of this issue together with diff for openide. text. Still I need to review the modules' code (as mentioned in the attached doc) e.g. JavaEditor. processAnnotations() etc.
Created attachment 11490 [details] Document describing the changes
Created attachment 11491 [details] Patch for openide to improve the locking
I would like to ask Jesse and Mato whether you could look at the fix and tell me any comments. Thanks in advance.
While I think it is good that the proposed patch uses the render lock more consistently, it seems to me that it does not go far enough in simplifying the threading and state transition model. There are many open issues in the associated HTML description. Consider requiring that *all* operations happen in the EQ, except (1) creating a new document not associated with any data object or view; (2) read-only operations inside a loaded document. What functionality would be inconsistent with such requirements? I can't think of any; and this would permit us to greatly simplify the threading. You would only need to wrap some r/o operations in the render lock, and everything else would behave in a natural single-threaded manner. Probably best to start a thread on nbdev; it is a bit cumbersome to discuss all the possibilities here, I think.
Jesse, my goal was rather to fix the current problems in openide.text without any incompatible changes so that we get rid of #33040 and similar ones. I would like to publish the resulting fixes in a nearest update of 3.5 so that people no longer experience the problematic exceptions. Requiring most of the operations to happen in EQ only is IMHO a semantical change for next NB version that should be documented in APIs. I apologize for misleading subject of this issue. I would like to change it to DEFECT with subject e.g. "Document locking issues in org.openide.text" if you don't mind. Having most of the work done in EQ is something I would likely vote for. Although certain operations over document can be lengthy (e.g. if you extend JPanel and wish to override all methods in the wizard the document modifications it takes about 5 seconds) it should generally be possible to break the lengthy operations down into pieces so that EQ is not blocked for long time. I'm not sure whether the item (2) would be feasible bcs once you post your readonly task from EQ (in which you have obtained the doc instance) into another e.g. RP thread then once you starting the readonly task you are in fact no longer sure whether the document was not closed in the meantime (you could of course check the change listener but it's additional complexity). So maybe (2) could happen in EQ too.
The patch seems OK to me. This should fix synchronization problems, so along with this also issue #33040 should be fixed. IMHO the issues like #23569 and #14803 may be also connected with this synchronization problem and could be also (partially) fixed along with this one. Because I am not such familiar with openide.text package code as its maintainer(s) or former maintainer(s), perhaps it would be fine, if the patch would be reviewed also by Peter Zavadsky or David Konecny.
Mila - agreed, I think the patch is fine and useful for correcting a number of clear deficiencies without introducing big semantic changes from the NB 3.5 behavior. (I did not look at it in detail - probably you now know this code better than anyone - but the description of what it does sounds good.) Especially if you have any intention of trying to correct these problems for a 3.5-based update, it would make sense to apply this patch in the trunk and get it tested for a while; I would probably want to make further changes in the trunk later but they can build on top of it. Feel free to adjust the summary of this issue; I will then open a fresh issue for longer-term review of general Editor API threading. Re. doing most things in EQ, and breaking up lengthy tasks such as you describe into chunks - I think this would be great if it works with acceptable responsiveness. I am already trying to prototype and measure this kind of behavior in an experimental sandbox to see if there are practical problems with it. Otherwise, even using the read/write locking permitted by render() + NbDocument.writeLock, I think you could reasonably stipulate that closing a document counts as a write and should thus be forbidden until all readers are done.
Changed issue summary to more suitable description.
With the applied patch I've been hitting issue 14803 so I've started digging into Annotations handling in the openide and corresponding part in the editor . I've filed and fixed issue 35914 and submitted a patch for editor part of the problem with annotations under issue 14803. Additionally I've made a related fix in DocumentLine and changed CloneableEditorSupport.updateLineSet() due to a problem with document reloading that I've found. Attaching updated diff.
Created attachment 11530 [details] Updated patch for openide.text
I will now cooperate with QA regarding testing of this and related patches. Hopefully there won't be any new deadlocks or other problems introduced by the patches.
Created attachment 11749 [details] Complete patch for trunk for openide.text threading issues and annotations
I have attached the complete patch for trunk. Unfortunately the handling in openide.text (mainly the annotations) influences the implementation of the annotations in the editor so the patch needs to be applied at once.
Checked into main trunk: Checking in editor/libsrc/org/netbeans/editor/Annotations.java; /cvs/editor/libsrc/org/netbeans/editor/Annotations.java,v <-- Annotations.java new revision: 1.13; previous revision: 1.12 done Checking in editor/libsrc/org/netbeans/editor/DrawEngine.java; /cvs/editor/libsrc/org/netbeans/editor/DrawEngine.java,v <-- DrawEngine.java new revision: 1.28; previous revision: 1.27 done Checking in editor/libsrc/org/netbeans/editor/DrawGraphics.java; /cvs/editor/libsrc/org/netbeans/editor/DrawGraphics.java,v <-- DrawGraphics.java new revision: 1.14; previous revision: 1.13 done Checking in editor/libsrc/org/netbeans/editor/Mark.java; /cvs/editor/libsrc/org/netbeans/editor/Mark.java,v <-- Mark.java new revision: 1.11; previous revision: 1.10 done Checking in editor/libsrc/org/netbeans/editor/MarkChain.java; /cvs/editor/libsrc/org/netbeans/editor/MarkChain.java,v <-- MarkChain.java new revision: 1.19; previous revision: 1.18 done Processing log script arguments... More commits to come... Checking in editor/src/org/netbeans/modules/editor/NbEditorDocument.java; /cvs/editor/src/org/netbeans/modules/editor/NbEditorDocument.java,v <-- NbEditorDocument.java new revision: 1.34; previous revision: 1.33 done Checking in editor/src/org/netbeans/modules/editor/NbToolTip.java; /cvs/editor/src/org/netbeans/modules/editor/NbToolTip.java,v <-- NbToolTip.java new revision: 1.8; previous revision: 1.7 done Processing log script arguments... More commits to come... Checking in java/src/org/netbeans/modules/java/JavaEditor.java; /cvs/java/src/org/netbeans/modules/java/JavaEditor.java,v <-- JavaEditor.java new revision: 1.143; previous revision: 1.142 done Processing log script arguments... More commits to come... Checking in openide/src/org/openide/text/CloneableEditorSupport.java; /cvs/openide/src/org/openide/text/CloneableEditorSupport.java,v <-- CloneableEditorSupport.java new revision: 1.96; previous revision: 1.95 done Checking in openide/src/org/openide/text/DocumentLine.java; /cvs/openide/src/org/openide/text/DocumentLine.java,v <-- DocumentLine.java new revision: 1.51; previous revision: 1.50 done Checking in openide/src/org/openide/text/NbDocument.java; /cvs/openide/src/org/openide/text/NbDocument.java,v <-- NbDocument.java new revision: 1.52; previous revision: 1.51 done Checking in openide/src/org/openide/text/PositionRef.java; /cvs/openide/src/org/openide/text/PositionRef.java,v <-- PositionRef.java new revision: 1.50; previous revision: 1.49 done
It appears that the fix produced one side effect problem described in issue 36369 which I have just fixed. I have updated a diff for 3.5 (that I maintain) accordingly. If there will be no further problems I'd like to integrate 3.5 diffs to release35_fixes too.
Created attachment 11912 [details] Complete patch file for [nevadafixes] branch
*** Issue 36868 has been marked as a duplicate of this issue. ***
I'm going to close this issue. Until now three problems related to this patch have appeared: issue 36369, issue 36601, issue 36624. All of them are fixed in trunk. Please reopen if any related deadlock or deficiency appears.
*** Issue 37109 has been marked as a duplicate of this issue. ***
*** Issue 37148 has been marked as a duplicate of this issue. ***
Created attachment 12246 [details] Complete diff for release35R branch
Integrated into release35R branch: Checking in editor/libsrc/org/netbeans/editor/Annotations.java; /cvs/editor/libsrc/org/netbeans/editor/Annotations.java,v <-- Annotations.java new revision: 1.12.30.1; previous revision: 1.12 done Checking in editor/libsrc/org/netbeans/editor/DrawGraphics.java; /cvs/editor/libsrc/org/netbeans/editor/DrawGraphics.java,v <-- DrawGraphics.java new revision: 1.13.30.1; previous revision: 1.13 done Checking in editor/libsrc/org/netbeans/editor/Mark.java; /cvs/editor/libsrc/org/netbeans/editor/Mark.java,v <-- Mark.java new revision: 1.10.30.1; previous revision: 1.10 done Checking in editor/libsrc/org/netbeans/editor/MarkChain.java; /cvs/editor/libsrc/org/netbeans/editor/MarkChain.java,v <-- MarkChain.java new revision: 1.18.30.1; previous revision: 1.18 done Processing log script arguments... More commits to come... Checking in editor/src/org/netbeans/modules/editor/NbEditorDocument.java; /cvs/editor/src/org/netbeans/modules/editor/NbEditorDocument.java,v <-- NbEditorDocument.java new revision: 1.33.30.1; previous revision: 1.33 done Checking in editor/src/org/netbeans/modules/editor/NbToolTip.java; /cvs/editor/src/org/netbeans/modules/editor/NbToolTip.java,v <-- NbToolTip.java new revision: 1.7.30.1; previous revision: 1.7 done Processing log script arguments... More commits to come... Checking in java/src/org/netbeans/modules/java/JavaEditor.java; /cvs/java/src/org/netbeans/modules/java/JavaEditor.java,v <-- JavaEditor.java new revision: 1.138.4.2.14.1; previous revision: 1.138.4.2 done Processing log script arguments... More commits to come... Checking in openide/src/org/openide/text/CloneableEditorSupport.java; /cvs/openide/src/org/openide/text/CloneableEditorSupport.java,v <-- CloneableEditorSupport.java new revision: 1.81.2.3.4.1; previous revision: 1.81.2.3 done Checking in openide/src/org/openide/text/DocumentLine.java; /cvs/openide/src/org/openide/text/DocumentLine.java,v <-- DocumentLine.java new revision: 1.47.108.2; previous revision: 1.47.108.1 done Checking in openide/src/org/openide/text/NbDocument.java; /cvs/openide/src/org/openide/text/NbDocument.java,v <-- NbDocument.java new revision: 1.51.42.1; previous revision: 1.51 done Checking in openide/src/org/openide/text/PositionRef.java; /cvs/openide/src/org/openide/text/PositionRef.java,v <-- PositionRef.java new revision: 1.48.56.1; previous revision: 1.48 done
Please set the Target Milestone when fixing bugs. I assume in this case promo-A is correct.
*** Issue 33040 has been marked as a duplicate of this issue. ***
Apologies. I've set Target Milestone to promo-A.
*** Issue 37426 has been marked as a duplicate of this issue. ***
*** Issue 36326 has been marked as a duplicate of this issue. ***
*** Issue 40272 has been marked as a duplicate of this issue. ***
*** Issue 42218 has been marked as a duplicate of this issue. ***
Verified.