Community
Participate
Working Groups
Build ID: M20080911-1700 Steps To Reproduce: On starting DSF GDB debugger, when any step in FinalLaunchSequence fails, the launch would fail but a zombie launch with a GDB process subnode remains active in the debug view. I saw the bug with Nokia DSF/GDB integration. But it can be easily reproduced with the standard DSF debugger by programmatically failing a step in FinalLaunchSequence. More information:
Created attachment 115640 [details] my patch to the bug In GDBLaunchDelegate, as FinalLaunchSequence is executed after ServicesLaunchSequence is executed, when FinalLaunchSequence runs into error, we should shutdown all services. My attached patch does that.
I agree that this is a severe error, but the proper way of handling the failure would be to implement the Step.rollBack() methods in the startup sequence. Unfortunately implementing rollBack is going to be very similar to implementing a shutdown sequence. So better yet, the startup and shutdown sequence implementations could be combined in a similar way that the initialization/shutdown sequence is implemented in GDBControl.
(In reply to comment #2) > I agree that this is a severe error, but the proper way of handling the failure > would be to implement the Step.rollBack() methods in the startup sequence. But what's involved here are two different sequences, ServicesLaunchSequence and FinalLaunchSequence. When a step in the second sequence fails, how can we rollback steps in first sequence ? > Unfortunately implementing rollBack is going to be very similar to implementing > a shutdown sequence. So better yet, the startup and shutdown sequence > implementations could be combined in a similar way that the > initialization/shutdown sequence is implemented in GDBControl. > I agree that's a better way of implementing the GDBLaunch.shutdownSession().
(In reply to comment #3) > (In reply to comment #2) > > I agree that this is a severe error, but the proper way of handling the failure > > would be to implement the Step.rollBack() methods in the startup sequence. > > But what's involved here are two different sequences, ServicesLaunchSequence > and FinalLaunchSequence. When a step in the second sequence fails, how can we > rollback steps in first sequence ? > > > Unfortunately implementing rollBack is going to be very similar to implementing > > a shutdown sequence. So better yet, the startup and shutdown sequence > > implementations could be combined in a similar way that the > > initialization/shutdown sequence is implemented in GDBControl. > > > > I agree that's a better way of implementing the GDBLaunch.shutdownSession(). > May be a good temporary workaround would be to just add a dummy first step to the startup sequence, which in its rollback implementation executes the whole shutdown sequence? In the longer term, I think it would be a worthwhile to combine the implementation of the two sequences.
BTW, in your patch you don't try to handle the ExecutionException to spot an error. Is this because no error is thrown?
(In reply to comment #4) > May be a good temporary workaround would be to just add a dummy first step to > the startup sequence, which in its rollback implementation executes the whole > shutdown sequence? I like that idea. Very clean. > In the longer term, I think it would be a worthwhile to combine the > implementation of the two sequences. Because we have two sequences for startup running one after the other, I think we'll still need something to cause the 'shutdown' of the combined ServicesLaunchSequence/ShutdownSequence. Note that there is nothing to cleanup in the FinalLaunchSequence itself. P.S. To easily test this, one can do a Local Attach launch and press the cancel button when the list of pids is given to the user. (Bug 233116)
*** Bug 234358 has been marked as a duplicate of this bug. ***
(In reply to comment #5) > BTW, in your patch you don't try to handle the ExecutionException to spot an > error. Is this because no error is thrown? > Do you mean possible error from ShutdownSequence ? It's already handled by "launch.shutdownSession()". And "launch.shutdownSession()" does not throw exception.
No I meant the startup sequence. I'm sorry but I got confused by the use of the finally() section. What I mean is that the launch should be completed only after the rollback (shutdown) is completed. So throwing the throwing the DebugException (after catching ExecutionException) should be delayed until the shutdown is completed. Moving the shutdown step to the rollback in the startup sequence should accomplish that.
(In reply to comment #9) > No I meant the startup sequence. I'm sorry but I got confused by the use of > the finally() section. What I mean is that the launch should be completed only > after the rollback (shutdown) is completed. So throwing the throwing the > DebugException (after catching ExecutionException) should be delayed until the > shutdown is completed. Throwing the DebugException in the catch block is actually happening after the "finally" block is done. > May be a good temporary workaround would be to just add a dummy first step to > the startup sequence, which in its rollback implementation executes the whole > shutdown sequence? Yes, it's a cleaner solution logically. But it imposes another problem/hardship for debugger like ours that extends the FinalLaunchSequence. In our debugger, I subclass FinalLaunchsequence by prepending two extra steps (downloading files and starting gdbserver) to those step in FinalLaunchSequece (with no copied code). With the dummy shutdown step having to be the first step, I have to _insert_ (instead of prepend) my extra two steps. That not only makes my code harder to write :), but more importantly it imposes a bad restriction (or makes it even harder) for any debugger that needs to extend FinalLaunchSequence. So before we have a better design of FinalLaunchSequence that is easy to extend, or even a better design of the whole startup sequence, I would propose we stick with my patch as a temporary solution for now.
(In reply to comment #10) > Throwing the DebugException in the catch block is actually happening after the > "finally" block is done. That's true. I forgot that the launch delegate doesn't execute on the dispatch thread. Still the call to GDBLaunch.shutdownSession() only initiates the shutdown sequence. Maybe the finally statement (or the catch()) statement should also use a query to make sure that the shutdown sequence is completed, before the launch delegate exits. > So before we have a better design of FinalLaunchSequence that is easy to > extend, or even a better design of the whole startup sequence, I would propose > we stick with my patch as a temporary solution for now. I'm fine with that, though I think it's more Marc's decision to make.
(In reply to comment #10) > But it imposes another problem/hardship for debugger like ours that extends the > FinalLaunchSequence. In our debugger, I subclass FinalLaunchsequence by > prepending two extra steps (downloading files and starting gdbserver) to those > step in FinalLaunchSequece (with no copied code). This is playing with fire (a little) :-), > With the dummy shutdown step having to be the first step, I don't think it must absolutely be the first step; it just needs to be before any step that can fail. If your two new steps can also fail, maybe your own first step could have its own rollback (which would somehow be skipped, if the original rollback has already been called.) (In reply to comment #11) > > So before we have a better design of FinalLaunchSequence that is easy to > > extend, or even a better design of the whole startup sequence, I would propose > > we stick with my patch as a temporary solution for now. > > I'm fine with that, though I think it's more Marc's decision to make. If the suggestion above is too much, then +1 on your patch. Until the FinalLaunchSequence is easier to override, I'm ok with the patch. I won't be able to get to it until mid next week, unless Pawel has time to take care of it, himself. Thanks.
(In reply to comment #12) > If the suggestion above is too much, then +1 on your patch. > Until the FinalLaunchSequence is easier to override, I'm ok with the patch. > I won't be able to get to it until mid next week, unless Pawel has time to take > care of it, himself. Thanks. I'm also pretty much booked. I assume you're talking about the patch itself. I think it's fair to put off any launch sequence refactoring until past 1.1, right?
(In reply to comment #12) > > With the dummy shutdown step having to be the first step, > > I don't think it must absolutely be the first step; it just needs to be before > any step that can fail. If your two new steps can also fail, maybe your own > first step could have its own rollback (which would somehow be skipped, if the > original rollback has already been called.) I don't think there is safe way to assume that a step may or may not fail. So the shutdown step must be the first step in the sequence that does nothing in execute() and just does the shutdown in rollBack(). Anyway, I've attached a patch that does the shutdown in a query per Pawel's this suggestion > Maybe the finally statement (or the catch()) statement > should also use a query to make sure that the shutdown sequence is completed, > before the launch delegate exits.
Created attachment 115866 [details] do shutdown in a query
I committed the fix. Marc please verify.
I find it weird that we can throw two exceptions out of the same method. If the finalLaunchSequence does not succeed we'll throw an exception, but in the finally clause, we can throw a second exception. The rest if fine. Verified.
Created attachment 117993 [details] Minor cleanup I also committed this trivial cleanup. Does this need to be marked as iplog?
(In reply to comment #17) > I find it weird that we can throw two exceptions out of the same method. If > the finalLaunchSequence does not succeed we'll throw an exception, but in the > finally clause, we can throw a second exception. That is kind of weird, obviously only one exception will be honored and I don't know which one! For our purposes it should be fine either way. (In reply to comment #18) > Created an attachment (id=117993) [details] > Does this need to be marked as iplog? No only patches submitted by non-committers need to be logged.
