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.
I've got a deadlock description from Jirka Skrivanek.
Created attachment 21457 [details] Deadlock thread dump
Although I admit that the BookmarkList (which features in the deadlock) is part of the new functionality added to the editor, I think the editor is not the main culprit here. The "Folder Instance Processor" thread asking for org.openide.text.CloneableEditorSupport.getOpenedPanes(CloneableEditorSupport.java:814) goes through javax.swing.JEditorPane.setEditorKit(JEditorPane.java:959) which is a swing code that does not have an exception of being called outside of the EDT but here it apparently is. As the EditorCookie.getOpenedPanes() does not state that it can be called in EDT only we probably cannot punish Hanz for doing that from the debugger. Not sure what should be the best solution here. Reassigning to openide/editor for further evaluation.
I'd guess it was caused by mklient's addition of multiviews. I don't like the fact that debugger actions try to enable/disable from different thread that AWT though.
Hanzi, could you enable/disable your actions from EDT (at least for this release)?
Debugger shouldn't update the action state from outside of AWT. The debugger code would call getCaret and so on from the same RP thread which is wrong as well.
What are you smoking guys? I am not updating state of action in that dump. I am not calling getCaret in this dump. Your evaluation is wrong.
Hanzi, sorry for our naivity, but when I saw e.g. org.netbeans.modules.debugger.projects.RunToCursorActionProvider.shouldBeEnabled(RunToCursorActionProvider.java:99) in the threaddump I was also mistakenly supposing that it's something related to debugger actions enabling. BTW if not then IMHO you should think about changing of naming of some of your methods to better reflect of what they are doing. I was looking into the CES.getOpenedPanes() and this problematic case is related to the state when a file was left opened prior shutdown of the IDE and when IDE gets restarted then there is a valid topcomponent but its JEditorPane was not yet constructed and it will be physically constructed (and the file contents loaded) when e.g. the user clicks on the editor tab or when someone asks for opened panes (which is the debugger in this case). This lazy opening behavior speeds up the IDE startup considerably and is desirable. Calling CE.getEditorPane() from CES.getOpenedPanes() was done as a bugfix of issue 23491 and there is the following comment in the code related to it: // #23491: pane could be still null, not yet shown component. // [PENDING] Right solution? TopComponent opened, but pane not. JEditorPane p = ed.getEditorPane(); AFAIK the original fix was about the fact that the (getOpenedPanes()[0] == null) in this case. The question is whether in this case no pane should be returned because in fact although the top component exists, the editor tab was not yet clicked by the user so the pane does not physically exist yet and the user could not see the pane's content yet. Is it even desirable that the call to getOpenedPanes() from the debugger will cause the editor pane to be initialized and loaded with the file's content? I know it's rather late to think about such things but I did not find any beter choice yet. Petre, what do you think?
The case is a bit more complicated. The change which calls getEditorPane()->initialize() was added with multiview support. Once the getOpenedPanes() get called, it will forward the call in the same thread. Even if it tried to forward the call to AWT, the code would still deadlock as getOpenedPanes would have to block. Not calling the method won't work at all after the multiview changes (and it is unrelated to the mentioned issue 23491) There are few possible workarounds of the problem: A) More conservative locking in bookmarks module + would allow debugger+editor to get through in the other thread - Still potential problems in debugger actions (subsequent calls to getCaret) B) Construction of shortcuts folder in AWT - won't probably help because the FolderInstance would have to wait to AWT and thus block as well C) Replanning of the debugger action enablement refresh asynchronously to AWT + Smallest and safest code change I thus believe that for 4.1, we should follow the path C.
*** Issue 57870 has been marked as a duplicate of this issue. ***
I do not agree with your evaluation. 1) A) is not workarround, it looks like correct fix. Thats the most important reason why we should follow this solution. 2) Actions should be crated in AWT thread, I think. Constructor should initialize action, and it is not a good solution to call my own methods in some invoke later sections form constructor. This is really strange rule. 3) C) Will not work, I think. I call getOpenedPanes () on many places. Fixing one of them is not correct solution. Generally said if you would like to change rules for getOpenedPanes () method, you should update JavaDoc for it, at least. This is incompatible change -> DevRev. And you should find all places in NB IDE where this method is called, and fix them.
> A) is not workarround, it looks like correct fix. See the last duplicate issue #57870. Very similar deadlock w/o bookmarks lock. Anyway, I'm not talking about the right fix here, only about the safest one for 4.1. I've already filed issue 57888 about the CloneableEditorSupport problem.
Per our discussion, Hanz, please fix in debugger. There's no time to experiment with potentially more correct solutions for 4.1. It seems that A) is not the correct fix either as issue #57870 doesn't include bookmarks at all.
BTW I have entered issue 57909 for the A) case.
I have applied workarround suggested by core team. But I have to remind you that its only very unreliable temporary patch for several "problems" in core & editor code. To be concrete: 1) Actions are created in "Folder Instance Processor" thread, in place of AWT thread. 2) Editor uses private monitor and calls "dangerous" code (loading of bookmarks - DataSystem - FolderInstance). 3) Editor loads list of all bookmarks during initialization of Main Window. These bugs should be fixed, so I am moving this bug back on core team to further analyse.
Posting the enablement code into AWT is imho correct way how to prevent the problem.
Do you have any tests? What I'm worried about is the comment from Mr. Jancura : "I can not guarantee that this deadlock is fixed by this change." so do I, I can not quarantee that this deadlock is fixed or not, BUT I trust to yarda and hope it is safe as jjancura wrote to the email on reviewers@netbeans.org . I tried the last(200504142205) NB4.1 build (open project, change the code, debug project, run project) and I haven't found any regressions - means fix is verified by QA.
I was not able to reproduce the deadlock on trunk build 200504141800. I did not find any consequences of the fix. Integrate the fix into 4.1.
fixed in release41 branch: no test provided. I have no idea how to do it. Yarda have some ideas - ask him, please. cvs commit -m "57622 Deadlock when FolderInstance RP calls pane.setEditorKit()" RunToCursorActionProvider.java (in directory C:\Hanz\Dev\release41\debuggerjpda\ant\src\org\netbeans\modules\debugger\projects) Checking in RunToCursorActionProvider.java; /cvs/debuggerjpda/ant/src/org/netbeans/modules/debugger/projects/RunToCursorActionProvider.java,v <-- RunToCursorActionProvider.java new revision: 1.11.24.1; previous revision: 1.11 done
verified in NB4.1(200504171930)
Why is it assigned to openide when the fix is in debugger? Why the action provider calls setEnabled in initializer? Is there any reason for this? Minor note - can we avoid non-static initializer?
> Why is it assigned to openide when the fix is in debugger? Have you read all comments? The bug is not fixed, but there is some ugly workarround. The bug is in core, and thats why its assigned to core. It should be fixed there. > Why the action provider calls setEnabled in initializer? Is there any reason for this? Why not? I have to initialize state of action. Initializer is the right place for doing initialization, isnt it?
if there is a problem in core (and editor) please file a new bugs. This one is closed now. re initialization: the main point is whether it is really needed to initialize enabled/disabled state. does anyone care at this moment?
The editor part is already filed as issue 57909. The most unfortunate thing however is that CloneableEditorSupport.getOpenedPanes() now needs to be called from the AWT thread only although its present javadoc does not state so - it's already filed as well - issue 57888. Debugger is sort of a victim of it but at this point it was the only place of a reasonably safe fix.