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.
The code in DebuggerManager requires that ACTION_START happen and complete synchronously. debuggercore then goes on to set the current session, start up models and views and so on. If, as the case is with dbx, and external debugger process has to be started it would be better if one could do all the starting work asynchronously and then notify debuggercore of the completion and only then have debuggercore adjust it's state.
I'd like this issue to be considered. what I get the start() callout on the AWT eventQ, I need to start a sub-process, and establish a network connection. Doing this on the AWT thread is a bad idea so I need to be able to return from start() quickly and declare the success of start() later. When I do, experimentally, return quickly there is a fair number of calls from debuggercore that require the connection to be established. I.e. I end up with races between debuggercore callbacks and the establishment of the connection. I'd rather avoid having to do things like collect all the callbacks in a queue and have them happen when a connection is established. Also the number of these callbacks is probably open-ended so I have to be over conservative. This is a lot of design and tricky multithreaded work which would be solved much easier if there was a startDone() which could be called back. The default implementation of start() would then call startDone() at the end for te existing synchronous semangcis.
The StartActionProvider in JPDA debugger also does some initializations asynchronously, so perhaps if you wrap the setup code and code that is executed on callbacks into synchronized blocks... But perhaps it's best to run the start in RequestProcessor. IMHO it could work fine if we put the connectorPanel.ok(); that is in org.netbeans.modules.debugger.ui.actions.ConnectAction.ConnectListener.actionPerformed() into a separate thread. But there would have to be some visual indication that the debugger is starting...
Note: "asynchronous => run in separate IDE thread" NOT! I'm using asynchronous as it applies to message passing. In the synchronoous style the caller calls a function and expects that upon return the side-effect of the function is fulfilled. In the asynchronous style the caller calls a function which might return almost immediately. Th ecaller makes no assumption. Instead a callback is made when the task is completed. This asynchrony leaves it up to the receiver as to how to proceed. They can do the work inline and right before returning call the callback. They can to the work in a separate IDE thread and call back upon completion. Or they can send a network message to an external engine and callback upon receiving an asynchronous return message from the network.
> The code in DebuggerManager requires that > ACTION_START happen and complete synchronously. Why do you mean so? Do you mean following code: k = engines.size (); for (i = 0; i < k; i++) { ((DebuggerEngine) engines.get (i)).getActionsManager ().doAction (ActionsManager.ACTION_START); } if (sessionToStart != null) setCurrentSession (sessionToStart); DebuggerEngine[] des = new DebuggerEngine [engines.size ()]; return (DebuggerEngine[]) engines.toArray (des); But it starts Engine only. It does not mean that your debugger implementation should be running (connected) in this time. Can you describe whats exactly your problem, please?
There is ActionsManager.postAction() that can be used for that purpose.
Yes, if I override postAction() I can make ACTION_START happen on the eventQ and avoid various races (see below for details). The point is I _don't_ want to run the start operation on the eventQ because we fork/exec to start dbx and that should not be done on the eventQ. But when we do run on the RP here's what happens: setCurrentSession() will fire a property change notification. There are _at least_ two side-effects of this: 1) debuggercore will call "debugger"group.open() which will call setModelsToView() which will call setModel() which will call TreeModel.addModelListener() which will attempt to contact dbx which may or may not be ready because the fork/exec is happening on the RP. Yes, this can be solved by "remembering" the addModelListener calls and sending stuff to dbx upon connection, but I'd rather not do that. 2) My own debuggerManager does a "session switch" ... and upon activating the switched-to session a bunch of stuff happens and if we don't have a successfull start this stuff fails. Yes, I can go bit by bit fix the session activation as well, but I'd rather not. So, it would seem that setCurrentSession() should rather be called right after doAction() in postAction() or better yet, as part of actionPerformedNotifier.run(). ------------------------------------------------------------------------ The above is the use-case of ACTION_START succeeding, but what if it fails? debuggercore doesn't seem to consider this case. A poorly started session lingers until the user finishes it. But then the engine/session be careful for all actions and other callbacks into them to check if the start was succesful or not and that is tedious. I'd rather see a failed start to not create a session. It will be up to each debugger implementation to post a dialog explaining that startup failed and why. One way to implement this is to have a new method startSucceeded(boolean success) to be called by the client. If 'success' is true then, and only then, setCurrentSession() should be called.
Well, I guess it's on you in which thread is the ACTION_START called. DebuggerManager.startDebugging() is called by JPDA debugger from our connection panel, or from ANT scripts. So I guess you have some launcher in C/C++ debugger as well. Then you determine whether it is on AWT EQ or not. The problem is, that one would expect that startDebugging() method waits for the ACTION_START to complete. But when I did that I got a deadlock in JPDA debugger (see #58911). This is why the easies fix was to keep the threading as it was. JPDA debugger is written that way - it starts while it's connecting and the user can stop the connection process (when it's connecting to nowhere) by the "Finish Debugger Session" action. I understand why this does not really suit to you, but JPDA debugger was simply written this way. We can agree to change that, which will imply that we'll have to also change the logic in JPDA debugger. I agree that it will be more intuitive design, but we need to do it carefully so that we'll not introduce new deadlocks. Since the original fix is not sufficient, I'm scheduling this for "Dev".
Fixed in trunk. The startup sequence waits for the asynchronous ACTION_START action, so that the session does not come up till it's fully started. /cvs/debuggercore/api/src/org/netbeans/api/debugger/ActionsManager.java,v <-- ActionsManager.java new revision: 1.20; previous revision: 1.19 /cvs/debuggercore/api/src/org/netbeans/api/debugger/DebuggerManager.java,v <-- DebuggerManager.java new revision: 1.23; previous revision: 1.22 /cvs/ant/debugger/src/org/netbeans/modules/ant/debugger/AntDebugger.java,v <-- AntDebugger.java new revision: 1.11; previous revision: 1.10 /cvs/debuggerjpda/ui/src/org/netbeans/modules/debugger/jpda/ui/ConnectPanel.java,v <-- ConnectPanel.java new revision: 1.24; previous revision: 1.23 /cvs/debuggerjpda/src/org/netbeans/modules/debugger/jpda/JPDADebuggerImpl.java,v <-- JPDADebuggerImpl.java new revision: 1.99; previous revision: 1.98 /cvs/debuggerjpda/ant/antsrc/org/netbeans/modules/debugger/jpda/ant/JPDAStart.java,v <-- JPDAStart.java new revision: 1.43; previous revision: 1.42 /cvs/debuggerjpda/src/org/netbeans/modules/debugger/jpda/actions/StartActionProvider.java,v <-- StartActionProvider.java new revision: 1.22; previous revision: 1.21
FYI: the fix of issue #75902 needs to be applied as well to prevent from a regression.
Martin, I don't understand how this fix works. I had thought that the crucial spot was in DebuggerManager.startDebugging() where ACTION_START is called and then setCurrentSession is called. setCurrentSession causes all kinds of side-effects so if ACTION_START runs asynchronously you get races. I don't see that your new code changes have changed anything.
The point is, that task.waitFinished() was added after ActionsManager.postAction(ActionsManager.ACTION_START); in DebuggerManager.startDebugging() method. Therefore setCurrentSession() is called *after* ACTION_START is finished. This works for asynchronous actions as well.
Martin, the code as it is now isn't helping me and I"m not sure it even makes sense. In ActionsManager a task is setup and a Runnable is created. The runnable is the actionPerformedNotifier parameter sent to postAction. I expected that setCurrentSession would be set from within this runnable, but it is not. Even though there is a call to task.waitFinished() It seems to not cause any waits. I found that when I call actionPerformedNotifier.run(), it will go and call task.actionDone() but by then we're way past task.waitFinished() and setCurrentSession() has been called. Then even if this worked ... isn't doing task.waitFinished() in the eventq a bad idea?
Currently DebuggerManager.getDebuggerManager ().startDebugging() is NOT called on AWT thread in JPDA. And it was designed to run synchronously, because we need to know when the debugger is started. E.g. we run progress.start() before call to startDebugging() and progress.finish() afterwards. I have also added a support for Thread.interrupt() to cancel the start action, so I guess we can not make this method asynchronous, there is no notification mechanism. So if you really need this, I would suggest to create a new method, that would behave asynchronously. Something like startDebuggingLazily(), which would return some task on which you could wait and which could be canceled. It's also not clear to me if you really need to call this method on AWT or not, since you wrote: "The point is I _don't_ want to run the start operation on the eventQ because we fork/exec ...". If you do not intend to run startDebugging() on AWT, then the current state should be sufficient IMHO. The task.waitFinished() should do the job, it finishes *after* you call actionPerformedNotifier.run() - at least this is how it works in JPDA. Please note, that task.waitFinished() is only in trunk, it's NOT in release55 branch. This might be the reason why it does not work for you.
A On release55 and it not working for me. I've actually been working off of trunk. We haven't finished our conversion to release_55_mars yet. B On "there is no notification mechanism". Then what is actionPerformedNotifier for? C You're going to get upset at the next comment :-) I don't want to run on the eventq for fork/exec, but I'm constrained to run on the eventq when I establish network connections. I'd love to eliminate that constraint but it's not trivial and I was going to do things bit by bit. I understand your suggestion of letting the action work synchronously on the RP and I'll look into it, but it'll have to work with the constraints of C. But then there is question B ...
A So it looks like it's necessary to backport the commit from "May 3 09:00:12". This will assure that setCurrentSession() is called after the ACTION_START is done - after actionPerformedNotifier.run() call. B I meant notification mechanism for DebuggerManager.startDebugging(DebuggerInfo) If this methods is made asynchronous, we need some notification that the debugger is actually started so that we can correctly display progress, etc. C :-) IMHO a solution is to add a method: Task DebuggerManager.startDebuggingLazily(DebuggerInfo), which would not wait for ACTION_START to finish. This can be easily added if it solves your threading issues.
Created attachment 37322 [details] Patch for release551 branch.
Review: The fix is safe enough for integration into NB 5.5.1
Resolving as fixed for now, so that the change in startup sequence can be ported to 551.
Thanks for the review, the fix is merged into release551 branch: /shared/data/ccvs/repository/debuggercore/api/src/org/netbeans/api/debugger/ActionsManager.java,v <-- ActionsManager.java new revision: 1.18.4.1.2.1.22.1; previous revision: 1.18.4.1.2.1 /shared/data/ccvs/repository/debuggercore/api/src/org/netbeans/api/debugger/DebuggerManager.java,v <-- DebuggerManager.java new revision: 1.21.4.1.2.1.22.1; previous revision: 1.21.4.1.2.1 /shared/data/ccvs/repository/debuggerjpda/ant/antsrc/org/netbeans/modules/debugger/jpda/ant/JPDAStart.java,v <-- JPDAStart.java new revision: 1.37.4.1.2.1.22.1; previous revision: 1.37.4.1.2.1 /shared/data/ccvs/repository/debuggerjpda/src/org/netbeans/modules/debugger/jpda/JPDADebuggerImpl.java,v <-- JPDADebuggerImpl.java new revision: 1.81.4.3.2.4.14.1; previous revision: 1.81.4.3.2.4 /shared/data/ccvs/repository/debuggerjpda/src/org/netbeans/modules/debugger/jpda/actions/StartActionProvider.java,v <-- StartActionProvider.java new revision: 1.18.2.2.2.1.22.1; previous revision: 1.18.2.2.2.1 /shared/data/ccvs/repository/debuggerjpda/ui/src/org/netbeans/modules/debugger/jpda/ui/ConnectPanel.java,v <-- ConnectPanel.java new revision: 1.18.4.2.2.1.22.1; previous revision: 1.18.4.2.2.1
Apologies for reopening this. I just got the opportunity to try startup sequences in asynchronous mode again and was surprised by .... the fact that DebuggerManager.startDebugging() uses task.waitFinished(0): for (i = 0; i < k; i++) { if (Thread.interrupted()) { break; } Task task = ((DebuggerEngine) engines.get (i)).getActionsManager (). postAction (ActionsManager.ACTION_START); if (task instanceof Cancellable) { try { task.waitFinished(0); } catch (InterruptedException iex) { if (((Cancellable) task).cancel()) { break; } else { task.waitFinished(); } } } else { task.waitFinished(); } } postAction will invariably() return a AsynchActionTask which is Cancellable. But waitFinished(0) returns instantly ... and the whole try/catch(InterruptedException) seems pointless. Perhaps waitFinished(0) was used because it throws InterruptedException and the assumption was made that 0 will wait forever?
http://java.sun.com/javase/6/docs/api/java/lang/Object.html#wait(long) "If timeout is zero, however, then real time is not taken into consideration and the thread simply waits until notified."
Task.waitFinished(0) is not obligated and in fact _doesn't_ work like Object.wait(0). There is a scenario where waitFinished(0) will return "instantly". Here is how it happens. call Task.waitFinished(0) on the eventQ. overridesTimeoutedWaitFinished() is false. So it create a Run and posts it on the RP. Fro here we have two threads in parallel: awt: waitFinished() recurses after posting the Run awt: overridesTimeoutedWaitFinished() is true awt: it calculates expectedEnd and goes into the loop and wait(0)'s ... this should hold "forever". rp: Task.run gets going and calls notifyRunning() rp: notifyRunning() does a notifyAll waking up ... awt: ... the wait called from waitFinished() which returns. awt: 'finished' is false so we don't return true. awt: however, because 'milliseconds' was 0 the (expectedEnd <= now) passes and we return true when we shouldn't.
To demonstrate my theory I "fixed" Task.waitFinished(long)" as follows: for (;;) { wait(milliseconds); if (finished) { return true; } if (milliseconds == 0) { System.out.printf("debuggercore *** milliseconds == 0\n" continue; } long now = System.currentTimeMillis(); if (expectedEnd <= now) { return false; } And things started working nicely. I also tried issuing DebuggerManager.startDebugging() on an RP. What I found was that waitFinished(0) sometimes worked correctly and sometimes didn't. I attribute this to different rates at which two RP's progress, the RP doing the wait and the RP used for Run in waitFinished().
I've submitted issue #130265 so that Task.waitFinished(0) is fixed. Thanks for discovering that.
Verified ... and Closing all issues resolved into NetBeans 6.7 and earlier.