Community
Participate
Working Groups
I noticed that when we cancel a launch, the GDB process is not killed (at least on Linux). This happens for an attach or postmortem session that we cancel from the dialog, but also when cancelling a launch using the progress view (if you can do it fast enough; I added a Thread.sleep(3000) to try it out). It seems that GDBBackend#destroy() does not call fProcess.destroy() because we are not in the right state. This even happens on the 7_0 branch!
Created attachment 191245 [details] Fix for HEAD The GDBBackend service code was a bit off. It has a thread monitoring the life of GDB, but that thread can be interrupted when we cleanup. The service was assuming that if the thread was dead, GDB was also dead, which was wrong. I've updated the service to dissociate the state of the monitoring thread and the state of GDB (the backend).
Committed to HEAD. Ling, can you review, since it is in the Backend service?
(In reply to comment #2) > Committed to HEAD. > > Ling, can you review, since it is in the Backend service? BTW, this bug seems to have been there from the very start of DSF-GDB, even before we introduced the Backend service.
*** cdt cvs genie on behalf of mkhouzam *** Bug 340021: GDB process not killed when canceling a launch [*] GDBBackend.java 1.21 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBBackend.java?root=Tools_Project&r1=1.20&r2=1.21
Hi, Marc, looks good overall. Two minor issues. 1. The following assertion assumes the MonitorJobStep.shutdown() is always done before GDBProcessStep.shutdown() in GDBBackend.shutdown(), which I don't think is very safe because GDBBackend.shutdown() could be overriden by a subclass which might not follow that order. // If we get here, our monitoring thread has been cleaned up already, // but GDB is running. We have to terminate GDB and send // the proper event. assert fMonitorJob.fMonitorExited == true; 2. This condition check in GDBProcessStep.shutdown() is good: if (GDBBackend.this.getState() == State.STARTED) ... but is it better it wraps the whole body of run() ? or even better we do the check at beginning of shutdown(), namely something like below ? protected void shutdown(final RequestMonitor requestMonitor) { if (getState() != State.STARTED) { // gdb not started yet or already killed, don't bother start a job... requestMonitor.done(); return; } new Job("Terminating GDB process.") { //$NON-NLS-1$ ...
(In reply to comment #5) > Hi, Marc, looks good overall. Two minor issues. > 1. The following assertion assumes the MonitorJobStep.shutdown() is always done > before GDBProcessStep.shutdown() in GDBBackend.shutdown(), which I don't think > is very safe because GDBBackend.shutdown() could be overriden by a subclass > which might not follow that order. > // If we get here, our monitoring thread has been cleaned > up already, > // but GDB is running. We have to terminate GDB and send > // the proper event. > assert fMonitorJob.fMonitorExited == true; That could happen. What I was trying to do with the assert was to make sure that the monitoring was really stopped when we send the BackendStateChangedEvent in that case. My worry was that if the monitoring thread was running, it would send a second BackendStateChangedEvent. It would probably be better to have an if check instead of an assert, and if the monitoring thread is exited, only then send the BackendStateChangedEvent. > 2. This condition check in GDBProcessStep.shutdown() is good: > if (getState() != State.STARTED) { > // gdb not started yet or already killed, don't bother start a > job... > requestMonitor.done(); > return; > } I like this. I'll do that. Looking at your suggestions made me wonder about multi-threading issues here. The getState() method and fBackendState, don't they need to be threadSafe, since they are accessed on jobs and not the executor? Or they should be accessed on the executor. I'll have to rework the fix for this.
(In reply to comment #6) > (In reply to comment #5) > What I was trying to do with the assert was to make sure that the monitoring > was really stopped when we send the BackendStateChangedEvent in that case. My > worry was that if the monitoring thread was running, it would send a second > BackendStateChangedEvent. It would probably be better to have an if check > instead of an assert, and if the monitoring thread is exited, only then send > the BackendStateChangedEvent. Another solution may be to go back to the old way, namely not send the event in GDBProcessStep.shutdown(), but always do it in MonitorJobStep like this: try { ... } catch () {...} finally { // send event } > Looking at your suggestions made me wonder about multi-threading issues here. > The getState() method and fBackendState, don't they need to be threadSafe, > since they are accessed on jobs and not the executor? Or they should be > accessed on the executor. I'll have to rework the fix for this. yes, we may need to use executor.
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > What I was trying to do with the assert was to make sure that the monitoring > > was really stopped when we send the BackendStateChangedEvent in that case. My > > worry was that if the monitoring thread was running, it would send a second > > BackendStateChangedEvent. It would probably be better to have an if check > > instead of an assert, and if the monitoring thread is exited, only then send > > the BackendStateChangedEvent. > > Another solution may be to go back to the old way, namely not send the event in > GDBProcessStep.shutdown(), but always do it in MonitorJobStep like this: > try { > ... > } > catch () {...} > finally { > // send event > } Well, that part of the original problem. The upon shutdown, the MonitoringJobStep only stops the monitoring job but not GDB, so the event should not be sent in that case. Of course, since we know it is a shutdown, we can assume that GDB will be shutdown, but before the current fix, it actually was not.
Re-opening to deal with multi-threading issue of the fix.
(In reply to comment #8) > ... > Well, that part of the original problem. The upon shutdown, the > MonitoringJobStep only stops the monitoring job but not GDB, so the event > should not be sent in that case. Of course, since we know it is a shutdown, we > can assume that GDB will be shutdown, but before the current fix, it actually > was not. Understood. Yes, with your fix, we can assume GDB and the monitor thread will die together.
Created attachment 191539 [details] Fix for Ling's comments I used the executor to handle multi-threading. I used an if statement instead of the assert. I moved the check for STARTED outside the job to avoid starting it. Committed to HEAD.
Ling, thanks for the good review and please let me know if you have other comments. Thanks
*** cdt cvs genie on behalf of mkhouzam *** Bug 340021: GDB process not killed when canceling a launch [*] GDBBackend.java 1.22 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBBackend.java?root=Tools_Project&r1=1.21&r2=1.22
