Community
Participate
Working Groups
Build Identifier: 20110615-0604 There are many cases that can make a gdb command hang (no response is received from target) which causes the IDE to wait forever for the command reply. Providing timeout for the commands will ensure better stability and useability for CDT. Check discussion http://dev.eclipse.org/mhonarc/lists/cdt-dev/msg23077.html Specifically http://dev.eclipse.org/mhonarc/lists/cdt-dev/msg23133.html & http://dev.eclipse.org/mhonarc/lists/cdt-dev/msg23136.html Reproducible: Always
Created attachment 205924 [details] Add timeout for gdb commands - Added timeout for all gdb commands - Added user preference to enable timeout and specify timeout value. Implemented timeout command listener in AbstractMIControl so that it applies to all gdb connections.
(In reply to comment #1) > Created attachment 205924 [details] > Add timeout for gdb commands > > - Added timeout for all gdb commands > - Added user preference to enable timeout and specify timeout value. > > > Implemented timeout command listener in AbstractMIControl so that it applies to > all gdb connections. I don't think AbstractMIControl needs to be changed (except NUM_OF_PARALLEL_COMMANDS constant). You can implement TimeOutCommandListener outside of it and add as a listener at startup. This will allow other debugger implementations use their own timeout policies. You may also consider setting the timeout value for individual launch configurations along with the global preference.
Mikhail, I am not sure I fully agree with your comments. Right now, we have GDBControl and GDBControl_7_0, and their inherit AbstractMIControl but are not related in any way. Putting timeout code in AbstractMIControl seems a reasonable approach. If you are worried about locking derived classes into specific policy, then probably AbstractMIControl should have a method called 'setupTimeoutPolicy' that can be overridden by derived classes to do something else (and disable this implementation), but it seems to me the current implementation is a reasonable default and so is good to have in the base class. Per-launch-configuration timeouts are possible, but not sure we *need* them. Mohamed, One concern I have about this patch (which I have already expressed offlist), is that we'll have possibly 3 threads devoted to waiting for command completion, which is wasteful on OS resources. Does anybody know a better way to implement timers in Eclipse/SWT?
(In reply to comment #3) > Mikhail, > > I am not sure I fully agree with your comments. Right now, we have GDBControl > and GDBControl_7_0, and their inherit AbstractMIControl but are not related in > any way. Putting timeout code in AbstractMIControl seems a reasonable approach. > If you are worried about locking derived classes into specific policy, then > probably AbstractMIControl should have a method called 'setupTimeoutPolicy' > that can be overridden by derived classes to do something else (and disable > this implementation), but it seems to me the current implementation is a > reasonable default and so is good to have in the base class. > I agree with you Vladimir, btw, there is already a protected addTimeOutCommandListener in AbstractMIControl that can be overridden to disable or change as needed. Please advise if you think the name should change. > > Mohamed, > One concern I have about this patch (which I have already expressed offlist), > is that we'll have possibly 3 threads devoted to waiting for command > completion, which is wasteful on OS resources. Does anybody know a better way > to implement timers in Eclipse/SWT? I wouldn't prefer to put swt code in core classes, but any advice for better implementation for the timers would be appreciated.
(In reply to comment #3) > Mikhail, > > I am not sure I fully agree with your comments. Right now, we have GDBControl > and GDBControl_7_0, and their inherit AbstractMIControl but are not related in > any way. Putting timeout code in AbstractMIControl seems a reasonable approach. > If you are worried about locking derived classes into specific policy, then > probably AbstractMIControl should have a method called 'setupTimeoutPolicy' > that can be overridden by derived classes to do something else (and disable > this implementation), but it seems to me the current implementation is a > reasonable default and so is good to have in the base class. > 'AbstractMIControl' already exists, there might be some extensions of it or its subclasses already. With your approach all these implementations have to be modified to prevent the timeout policy working. What I am suggesting is to implement the timeout policy as a separate class and really don't understand what's wrong with that. As an example of a similar approach see 'StepController' class. I'm not suggesting to use the adapter mechanism in our case, but it still worth to look at. Anyway, it's up to Marc to decide which approach is best in this case.
(In reply to comment #5) > (In reply to comment #3) > > Mikhail, > > > > I am not sure I fully agree with your comments. Right now, we have GDBControl > > and GDBControl_7_0, and their inherit AbstractMIControl but are not related in > > any way. Putting timeout code in AbstractMIControl seems a reasonable approach. > > If you are worried about locking derived classes into specific policy, then > > probably AbstractMIControl should have a method called 'setupTimeoutPolicy' > > that can be overridden by derived classes to do something else (and disable > > this implementation), but it seems to me the current implementation is a > > reasonable default and so is good to have in the base class. > > > > 'AbstractMIControl' already exists, there might be some extensions of it or its > subclasses already. With your approach all these implementations have to be > modified to prevent the timeout policy working. > What I am suggesting is to implement the timeout policy as a separate class and > really don't understand what's wrong with that. As an example of a similar > approach see 'StepController' class. I'm not suggesting to use the adapter > mechanism in our case, but it still worth to look at. > Anyway, it's up to Marc to decide which approach is best in this case. [mhussein] I do understand your concern that there might be other classes outside CDT code that extends AbstractMIControl and not GDB. For those classes the timeout should be opt-in not opt-out i.e., their behavior shouldn't change without them making code changes. I just think that the consensus in the discussion was that timeout is needed in all cases. If You, Vladimir, Marc, ... think that there might be extenders of AbstractMIControl that shouldn't get this, then I agree with you we should change the code. We can simply make the protect method in AbstractMIControl empty (or remove it altogether) and add two methods in GDB control classes while moving the TimeOut class as a separate file. Please let me know if I should make this change and post an updated patch.
(In reply to comment #5) > 'AbstractMIControl' already exists, there might be some extensions of it or its > subclasses already. With your approach all these implementations have to be > modified to prevent the timeout policy working. So, do we think vendors extending AbstractMIControl would like to automatically get the new timeout feature, or do we think they would prefer to not get it by default? Mikhail has much more experience with such situations, so I trust what he recommends. > What I am suggesting is to implement the timeout policy as a separate class AbstractMIControl is pretty complex already, so dealing with the timeout as a separate class is a good idea. > As an example of a similar > approach see 'StepController' class. I'm not suggesting to use the adapter > mechanism in our case, but it still worth to look at. That is a great idea (if I understand it correctly :-))! Do you mean to have a a separate class monitoring the timeout and when it happens it would issue a new event e.g., CommandTimedOutEvent (similar to SteppingTimedOutEvent). That way, different classes could easily be made aware of that situation and decide what to do. Another concern that came to my mind was all-stop mode. In all-stop mode, it happens that commands are sent to GDB wile the inferior is running. It is probably a bug when we do such a thing, but it does happen. In that case, because of all-stop mode, there is no reply until the inferior is interrupted. This will cause a timeout. Calling 'shutdown()' in such cases may not be such a good idea. Any opinions on that case?
Minor comments on the patch (ignoring AbstractMIControl since it will change): - Please update copyright notice for every file you modified. IGdbDebugPreferenceConstants - Can you put your changes above the line defining PREFIX? (which really should be at the top) - Can you change the new prefs to be: public static final String PREF_COMMAND_TIMEOUT_ENABLE = "commandTimeoutEnable"; //$NON-NLS-1$ public static final String PREF_DEFAULT_COMMAND_TIMEOUT = "defaultCommandTimeout"; //$NON-NLS-1$ ? Should we be using PREFIX before all the preference strings? This is probably a bug from the very start, but we can't change it now because of backwards compatibility. MessagesForPreferences - could you add /** @since 2.3 */ before each new member? The API tooling does not give a warning because the class in insternal, but it is nicer to have it. GdbDebugPreferencePage - Can you make the allowed maximum timeout bigger than 1000, how about 1000000?
Created attachment 206262 [details] Minor update to handle Marc's code comments Attached is a minor update to handle Marc's code comments. I will work on refactoring the code according to (my understanding) of Mikhail's proposal and send an updated patch later. (In reply to comment #8) > Minor comments on the patch (ignoring AbstractMIControl since it will change): > > - Please update copyright notice for every file you modified. > [mhussein] Done. > IGdbDebugPreferenceConstants > - Can you put your changes above the line defining PREFIX? (which really should > be at the top) > - Can you change the new prefs to be: > public static final String PREF_COMMAND_TIMEOUT_ENABLE = > "commandTimeoutEnable"; //$NON-NLS-1$ > public static final String PREF_DEFAULT_COMMAND_TIMEOUT = > "defaultCommandTimeout"; //$NON-NLS-1$ > [mhussein] Done. > ? Should we be using PREFIX before all the preference strings? This is > probably a bug from the very start, but we can't change it now because of > backwards compatibility. > [mhussein] I have moved PREFIX to top, and used it in the pref names I have added a comment to PREFIX to indicate it should be used and why it isn't. > MessagesForPreferences > - could you add /** @since 2.3 */ before each new member? The API tooling does > not give a warning because the class in insternal, but it is nicer to have it. > [mhussein] Done. > GdbDebugPreferencePage > - Can you make the allowed maximum timeout bigger than 1000, how about 1000000? [mhussein] Done, though 100000s is > 1 day, I guess that would be a bit too much for timeout for a single command :)
Created attachment 206328 [details] Implement fix according to Mikhail's suggestion Please find attached a new patch using Mikhail's suggestion (in comment #5): - Moving TimeoutCommandListener to separate file. - Call it using a step in the FinalLaunchSequence instead of directly Please let me know if you have any further comments. One remaining point is Marc's question in comment #7 regarding whether calling shutdown is the right approach. I guess this needs confirmation from Mikhail.
(In reply to comment #9) Thanks for the update. > (In reply to comment #8) > > Minor comments on the patch (ignoring AbstractMIControl since it will change): > > > > - Please update copyright notice for every file you modified. > > > [mhussein] Done. You should also put your name in the copyright of MessagesForPreferences.properties > > GdbDebugPreferencePage > > - Can you make the allowed maximum timeout bigger than 1000, how about 1000000? > [mhussein] Done, though 100000s is > 1 day, I guess that would be a bit too > much for timeout for a single command :) That is just the maximum allowed. I don't expect someone to set a timeout that high, but no need to prevent it either. I'll let Mikhail comment on the overall Timeout solution, but here are some comments on DSF issues: FinalLaunchSequence.java - I don't think this is the right place to create the listener. I'd like to know what Mikhail had in mind for that. - Don't pass RequestMonitor to the TimeOutCommandListener, that RM will be completed in the FinalLaunchSequence. Instead, in TimeOutCommandListener, create a new RM as is done in ShutdownSequence.java TimeOutCommandListener.java - Do we need to create a new ScheduledThreadPool? Instead, can't we use the DSF executor as is done in SteppingController? - Don't pass in IGDBControl, pass in DsfSession instead and use a DsfServicesTracker to get the IGDBControl service (like SteppingController) - if a timeout expires, it should be removed from the 'commands' map. It may not matter since shutdown() is being called, but if we change that, the map will grow unnecessarily. - Do we want to handle the timer preference and timer enable preference changing during a running debug session? What will the user expect it to do if the preference is changed during a session? - when calling connectionControl.shutdown() it should be done on the DSF executor. In this case, it may not be a problem because it seems to be ThreadSafe, but it may not be in the future. So, whenever you call a DSF service it must be using the DSF executor. If you create the timer using the DSF executor, does it mean the callback runs on the DSF executor too? If so, then using the DSF executor to create the timer will be enough. Please confirm if that is true.
I'll take a look.
I've played with this patch (to be honest, inside our product tree, not inside CDT git head), and run into couple problems: 1. When timeout fires during initial connection, we get error message saying "Connection is shut down". This is pretty confusing, and it's complicated by the point that "gdb traces" console goes away at this point. Can we inform the user that the timeout happened? 2. It seems that somehow, the preference is stored inside UI plugin preferences, while AbstractMIControl tries to read them from core plugin preferefences, so essentially the preference does not have any effect. HTH, Volodya
Created attachment 208645 [details] Improved patch In this patch contains a new service that monitors the execution of GDB/MI commands. When a command is timed out the command timeout policy registered with the service is notified. The default command timeout policy sets the command status to ERROR, terminates the session and notifies the user. The UI for timeout settings is added to "C/C++->Debug->GDB" preference page. It includes the ability to change the timeout values or disable timeouts for selected GDB commands.
Marc, could you please look at this patch? It is much bigger than I expected. The patch contains some API extensions and also registers a generic status handler to report the core errors to users which can be used in other cases.
(In reply to comment #15) > Marc, could you please look at this patch? It is much bigger than I expected. > The patch contains some API extensions and also registers a generic status > handler to report the core errors to users which can be used in other cases. Thanks for the patch. I like the fact it uses a new service. I can have a look once back from holidays. I hope that is ok.
(In reply to comment #16) > (In reply to comment #15) > > Marc, could you please look at this patch? It is much bigger than I expected. > > The patch contains some API extensions and also registers a generic status > > handler to report the core errors to users which can be used in other cases. > > Thanks for the patch. I like the fact it uses a new service. > > I can have a look once back from holidays. I hope that is ok. Thanks Marc. Take your time, I'll be on holiday too.
Mikhail, can you attach the icon: adv_to_settings_wiz.png. Until then I'm using a fake one, copied from the existing icons.
The patch "Improved patch" does not apply cleanly to master. GdbDebugPreferencePage and GdbDebugServicesFactory have collisions. I can fix the second one by hand, but the GdbDebugPreferencePage is a 600-line diff... I thought that using Git, I could work around the problem by applying the patch to the parent commit used to create the patch. Problem is, the commit id in the patch is not the parent, but the actual commit, which does not help me. I don't know enough about Git I guess.
(In reply to comment #19) > The patch "Improved patch" does not apply cleanly to master. > GdbDebugPreferencePage and GdbDebugServicesFactory have collisions. I can fix > the second one by hand, but the GdbDebugPreferencePage is a 600-line diff... > > I thought that using Git, I could work around the problem by applying the patch > to the parent commit used to create the patch. Problem is, the commit id in > the patch is not the parent, but the actual commit, which does not help me. > I don't know enough about Git I guess. Thanks for looking at the patch. I will update it, need to make some other changes anyway.
Created attachment 210213 [details] Updated patch. Updated the previous patch against the latest HEAD.
Created attachment 210215 [details] The dialog image.
Additional comments: 1. The patch defines a new status handler for the DSF/GDB plugins. It would better to re-use the status handler from org.eclipse.cdt.debug.ui but the status handler codes are internal. 2. We need a way to find out whether the launcher is still running or not. The launcher has its own error reporting mechanism and we get two error dialogs if an error occurs when the launch sequence is executed. The patch defines and implements a new interface 'IGDBControlExtension' for this purpose. These two cases are not timeout specific and can be extracted into separate patches.
Hi Mikhail, I'm not sure if I'll have time to have a look this week, but I should next week. I hope that is ok.
(In reply to comment #24) > Hi Mikhail, > > I'm not sure if I'll have time to have a look this week, but I should next > week. I hope that is ok. It's OK with me. I'll try to keep the patch up-to-date.
Created attachment 210604 [details] Updated patch. Keeping the patch up-to-date.
(In reply to comment #26) > Created attachment 210604 [details] > Updated patch. > > Keeping the patch up-to-date. I'll schedule some time in my February sprint for this review.
I've gone through half the patch and I like it a lot. Nice work. The StatusHandler stuff is nice and could be useful again, as you point out. I ran across some exceptions when trying to test things and I thought I would report them right away, while I continue looking at the patch. To reproduce: 1- run the test eclipse with assertions (-ea in the VM arguments of the launch) 2- In AbstractMIControl.processMIOutput, add as the first line: if (line.indexOf("register-names") != -1) return; The idea is to ignore the reply of a register command to test the timeout. Please let me know if there is a more elegant way. 3- launch a GDB 7.4 session (not sure the version matters) 4- bring the registers view to the front. java.lang.IllegalStateException: RequestMonitor: 38^error,msg="Command '-data-list-register-names' is timed out", done() method called more than once at org.eclipse.cdt.dsf.concurrent.RequestMonitor.done(RequestMonitor.java:285) at org.eclipse.cdt.dsf.debug.service.command.CommandCache$1.done(CommandCache.java:402) at org.eclipse.cdt.dsf.mi.service.command.AbstractMIControl.stopCommandProcessing(AbstractMIControl.java:264) at org.eclipse.cdt.dsf.gdb.service.command.GDBControl_7_0.eventDispatched(GDBControl_7_0.java:352) and java.lang.AssertionError: Exception thrown by a IServiceEventListener.ServiceHandlerMethod method at org.eclipse.cdt.dsf.service.DsfSession.doDispatchEvent(DsfSession.java:529) at org.eclipse.cdt.dsf.service.DsfSession.access$2(DsfSession.java:463) at org.eclipse.cdt.dsf.service.DsfSession$3.run(DsfSession.java:390) at org.eclipse.cdt.dsf.concurrent.DefaultDsfExecutor$TracingWrapperRunnable.run(DefaultDsfExecutor.java:374)
I'm not sure if it would be possible to write some JUnit tests for this feature, but if it is, it would make it easier to test :)
Thanks for looking at the patch. (In reply to comment #29) > I'm not sure if it would be possible to write some JUnit tests for this > feature, but if it is, it would make it easier to test :) Actually, I have been thinking about it. I'm not 100% sure but think the timeout service can be tested separately without running a session. This would require to write a class that generates commands and register the service as a listener. It may be hard to implement but I'll think about it.
I noticed that when a command times out, we apparently terminate the debug session twice. GdbCommandTimeoutService.processTimedOutCommand() 1- calls the new timeoutPolicy class, which will call IGDBControl.terminate 2- sends a CommandTimedOutDMEvent, which is caught by GdbLaunch which then calls shutdownSequence These two actions both terminate the debug session. Do we really want to do both?
Created attachment 210934 [details] Updated patch. (In reply to comment #28) > I've gone through half the patch and I like it a lot. Nice work. The > StatusHandler stuff is nice and could be useful again, as you point out. > > I ran across some exceptions when trying to test things and I thought I would > report them right away, while I continue looking at the patch. > > To reproduce: > 1- run the test eclipse with assertions (-ea in the VM arguments of the launch) > 2- In AbstractMIControl.processMIOutput, add as the first line: > if (line.indexOf("register-names") != -1) return; > The idea is to ignore the reply of a register command to test the timeout. > Please let me know if there is a more elegant way. I have been using two tricks to do the testing. First, I set the timeout value of the GDB command I want to test to "1". The second is similar to yours: add a "stop" flag to RxThread and set from TxThread when the tested command is issued. > 3- launch a GDB 7.4 session (not sure the version matters) > 4- bring the registers view to the front. > > > java.lang.IllegalStateException: RequestMonitor: 38^error,msg="Command > '-data-list-register-names' is timed out", done() method called more than once > at > org.eclipse.cdt.dsf.concurrent.RequestMonitor.done(RequestMonitor.java:285) > at > org.eclipse.cdt.dsf.debug.service.command.CommandCache$1.done(CommandCache.java:402) > at > org.eclipse.cdt.dsf.mi.service.command.AbstractMIControl.stopCommandProcessing(AbstractMIControl.java:264) > at > org.eclipse.cdt.dsf.gdb.service.command.GDBControl_7_0.eventDispatched(GDBControl_7_0.java:352) > > and > > java.lang.AssertionError: Exception thrown by a > IServiceEventListener.ServiceHandlerMethod method > at > org.eclipse.cdt.dsf.service.DsfSession.doDispatchEvent(DsfSession.java:529) > at org.eclipse.cdt.dsf.service.DsfSession.access$2(DsfSession.java:463) > at org.eclipse.cdt.dsf.service.DsfSession$3.run(DsfSession.java:390) > at > org.eclipse.cdt.dsf.concurrent.DefaultDsfExecutor$TracingWrapperRunnable.run(DefaultDsfExecutor.java:374) I fixed the first exception. The timed out command should be removed from fRxCommands. Otherwise it will be processed either by "stopCommandProcissing()" when the session is terminated or by "RxThread.processMIOutput()" if the timeout value is to small. With this patch I can reproduce the assertion error. I've seen it it without it a couple of times but not sure if it is related or not - can not reproduce it consistently. (In reply to comment #31) > I noticed that when a command times out, we apparently terminate the debug > session twice. > > GdbCommandTimeoutService.processTimedOutCommand() > 1- calls the new timeoutPolicy class, which will call IGDBControl.terminate > 2- sends a CommandTimedOutDMEvent, which is caught by GdbLaunch which then > calls shutdownSequence > > These two actions both terminate the debug session. Do we really want to do > both? Sorry, I had been trying both approaches and forgot to remove the part that shuts down the session depending on CommandTimedOutDMEvent. I still think that we should fire an event, so clients can do something else if they want. I'm just not sure whether we should fire the event before calling in timeout policy or after as it is now. There is a bigger issue with the patch though. "list-features' command is issued before the command control service is initialized and can not be caught by the timeout service. This issue can be solved by rewriting the patch and moving some parts of it in "AbstractMIControl" class which I have been trying to avoid. At the same time I don't understand why the features list has to be created on the initialization stage. Services that use it can request it dynamically. Am I missing something?
(In reply to comment #32) Corrections: > I fixed the first exception. The timed out command should be removed from > fRxCommands. Otherwise it will be processed either by "stopCommandProcissing()" > when the session is terminated or by "RxThread.processMIOutput()" if the > timeout value is to small. Should be "TOO small". > With this patch I can reproduce the assertion error. I've seen it it without it > a couple of times but not sure if it is related or not - can not reproduce it > consistently. Should be "can NOT reproduce".
(In reply to comment #32) > Created attachment 210934 [details] > Updated patch. Thanks > (In reply to comment #31) > > I noticed that when a command times out, we apparently terminate the debug > > session twice. > > > > GdbCommandTimeoutService.processTimedOutCommand() > > 1- calls the new timeoutPolicy class, which will call IGDBControl.terminate > > 2- sends a CommandTimedOutDMEvent, which is caught by GdbLaunch which then > > calls shutdownSequence > > > > These two actions both terminate the debug session. Do we really want to do > > both? > > Sorry, I had been trying both approaches and forgot to remove the part that > shuts down the session depending on CommandTimedOutDMEvent. I still think that > we should fire an event, so clients can do something else if they want. It is not clear to me why use a new ICommandTimeoutPolicy, if the ICommandTimedOutDMEvent can provide the same ability? Wouldn't it be simpler to just use the event? We can just make sure that we process that event in an easy to override location of code. > I'm just not sure whether we should fire the event before calling in timeout > policy or after as it is now. I guess this illustrates the conflict in having two 'competing' solutions :) Another design question I'm struggling with is: Why have an new DSF service for the CommandTimeout feature? I know I said I liked the idea :), but looking at what the service does, I don't think we really need a service, do we? We could have a GdbCommandTimeout class be a simple class instantiated by the IGDBControl service itself. Or by some other class during the launch. Maybe like the SteppingController, if it makes sense. > There is a bigger issue with the patch though. "list-features' command is > issued before the command control service is initialized and can not be caught > by the timeout service. Nice catch, I had not noticed that. > This issue can be solved by rewriting the patch and > moving some parts of it in "AbstractMIControl" class which I have been trying > to avoid. > At the same time I don't understand why the features list has to be created on > the initialization stage. Services that use it can request it dynamically. Am I > missing something? Quickly looking at Bug 322658, and trying to remember the details, I think I felt it was good to fetch the feature list early as it may be needed for the initialization of another service. For example, if the feature list indicates that GDB does not support, say, Tracepoints, we could choose not to instantiate the Tracepoint service at all. This point may be addressed automatically by the solution to the "why a service?" question. So, let's keep it until we settle that other one.
I pretty much went over the entire patch, here are the bigger questions I have (including the two previous ones from comment 34). Once again, this feature has some complexities I hadn't foreseen. But the proposed solution is nice and elegant. 1- ICommandTimeoutPolicy vs ICommandTimedOutDMEvent, could we just use the event? 2- Should GdbCommandTimeoutService be a service or just a class? 3- Just an idea: instead of adding the IGDBControlExtension API to know if the CommandControl service has been completely initialized, we could have DefaultGdbCommandTimeoutPolicy be a @DsfServiceEventHandler and listen for DataModelInitializedEvent, that event would indicate that the initialization is finished. I'm not saying it is a better way, but it would keep the change more local to the timeout feature and avoid a new interface. 4- Do you want to support the usecase where the user only wants a timeout on some commands, but all others have no timeout? In that case, the user would need to set the default timeout to 0, while setting some custom timeouts. Currently, the preference page does not allow to set the default timeout to 0. If you want to support that case, there may be some changes needed in the GdbCommandTimeoutService, where we may need to check for waitTimeout>0 in calculateWaitTimeout() 5- I think the TimerThread algorithm does not cover every case. If I understand correctly, it only checks the first command of the queue, although any of the next commands may have a shorter timeout. These logs show the problem: [235,484] Command '-environment-cd /home/lmckhou/runtime-TestDSF/DSFTestApp' sent, timeout = 1000000 <==== first command in queue has long timeout 235,491 [MI] 2-environment-cd /home/lmckhou/runtime-TestDSF/DSFTestApp 235,492 [MI] 2^done <===== ignored to fake a timeout [235,546] Command '-list-thread-groups' sent, timeout = 2000 <==== second command in queue has a shorter timeout 235,546 [MI] 3-list-thread-groups 235,547 [MI] 3^done,groups=[{id="i1",type="process"}] <==== ignored [237,441] Processing command '-environment-cd /home/lmckhou/runtime-TestDSF/DSFTestApp', command timeout is 1000000 [237,442] Setting timeout 2000 for command '-environment-cd /home/lmckhou/runtime-TestDSF/DSFTestApp' [239,442] Processing command '-environment-cd /home/lmckhou/runtime-TestDSF/DSFTestApp', command timeout is 1000000 [239,442] Setting timeout 2000 for command '-environment-cd /home/lmckhou/runtime-TestDSF/DSFTestApp' [241,443] Processing command '-environment-cd /home/lmckhou/runtime-TestDSF/DSFTestApp', command timeout is 1000000 [241,443] Setting timeout 2000 for command '-environment-cd /home/lmckhou/runtime-TestDSF/DSFTestApp' As you can see, the timeout of 2000ms is not triggered because the algo keeps looking at the first command of the queue with a timeout of 1000000ms. I'm thinking that using a timer for each command would be much easier to code. But maybe that would create too many timers? I don't think we'll have that many outstanding commands though, so it may be ok. 6- AbstractMIControl only sends out 3 commands to GDB at a time, the other commands are kept in the queue until we get an answer for at least one of the 3 sent commands. Imagine this scenario: a) long default timeout set b) short timeout for register commands c) the three sent commands are not getting an answer. Because of the long timeout, there is no error happening yet d) user opens the register view e) register command are not sent out to GDB because 3 commands are pending => short timeout is not triggered because the commands are not sent to GDB. I admit this is a case that will probably never happen, but it illustrates that the timer for a command should probably be started when the command is queue instead of when it is sent. I haven't tried it, but what do you think about moving the code from GdbCommandTimeoutService.commandSent() to GdbCommandTimeoutService.commandQueued()? 7- could you explain to me the reasons we need AbstractMIControl.commandFailed()? It makes sense, but I don't think I grasp all the reasons behind it. 8-The new Advanced preference page is great. However we need to help the user know how to specify the MI command. It was not clear to me how to specify it, although now that I know, it is kind of obvious. How about an example at the top of the preference page? "example: -thread-info" 9- Can we make the trace logs more uniform? i.e., remove [] and add a prefix such as [TMO], like AbstractMIControl.MI_TRACE_IDENTIFIER Currently it looks like: 326,383 [MI] 41-var-create --thread 1 --frame 0 - * um3 [326,384] Command '-var-create - * um4' sent, timeout = 1000 Other DSF trace logs follow this pattern I have some minor comments about code that may disappear, depending on the decisions you make about the above questions, so I won't post them until then. Thanks for the patch!
Marc, let me explain what I was trying to achieve in my previous patch. My intention was to implement the timeout functionality only for GDB based debuggers. Other backends may support the timeout internally and do not require an external implementation of it. I assume one of these backends is an MI based non-GDB debugger since there is a separation in DSF/GDB between GDB and MI. 'AbstractMIControl' seems to be one of the classes that have some non-GDB features. Is this correct? I agree, the timeout implementation in the previous patch is not a "real" service. But implementing it as a service provides the flexibility I am looking for. Anyone can replace or disable it without modifying other services. Another question is what do we want to do when a command is timed out. First of all we need to process the command as if an error occurred in GDB, i.e. mark the command as "timed out". If a timeout occurred when the launch sequence is executed the launch will be terminated. If a timeout occurred after the session is launched an extra action may be required. Here is where I use "ICommandTimeoutPolicy". The default implementation just terminates the session, but some debuggers can provide their own implementation. I also fire 'ICommandTimedOutDMEvent' which may be unnecessary, those implementations that need it can define their own timeout policies but I thought it would be good if we provided it. These are the ideas behind my implementation. I can just move the implementation into 'GDBControl*' or even in 'AbstractMIControl' class, fix the algorithm and other minor issues if you think the previous patch is too complicated.
(In reply to comment #36) > Marc, let me explain what I was trying to achieve in my previous patch. > > My intention was to implement the timeout functionality only for GDB based > debuggers. Other backends may support the timeout internally and do not require > an external implementation of it. > I assume one of these backends is an MI based non-GDB debugger since there is a > separation in DSF/GDB between GDB and MI. 'AbstractMIControl' seems to be one > of the classes that have some non-GDB features. Is this correct? Originally, we kept classes that were for MI in one location while having GDB-specific parts in other classes. This was because WindRiver had their own backend that spoke MI. I don't believe that is an important use-case for them anymore though. In fact, IIRC, Pawel mentioned that this division of classes turned to be un-needed while complicating the code a lot. We'd have to check with him and the list if someday we want to merge the classes - I'd like that a lot as it would make the code simpler - although it would be a drastic API change. Anyway, we're not at that point yet :) To make a long story short, officially, AbstractMIControl should not contain GDB-specific code, as you very perceptively deduced. > I agree, the timeout implementation in the previous patch is not a "real" > service. But implementing it as a service provides the flexibility I am looking > for. Anyone can replace or disable it without modifying other services. It is unclear to me why using a service makes it easier to replace instead of using a method in GDBControl/GDBControl_7_0 (or another service) along the lines of the pseudo-code: protected createCommandTimeout() { return new GdbCommandTimeoutService(); } In fact, for those that already override the GDBControl service, it will be very easy to turn off the timeout. Do you foresee changes in the CommandTimeout code that will later on require it to be a service? Then it would make sense to be proactive. This being said, if you really feel the service solution is the best one, I'm ok with your decision. > Another question is what do we want to do when a command is timed out. First of > all we need to process the command as if an error occurred in GDB, i.e. mark > the command as "timed out". If a timeout occurred when the launch sequence is > executed the launch will be terminated. I like the idea of marking the command as "timed out" as it allows to cleanly recover, if we ever choose not to terminate the entire session. What happens if the command does arrive, but after the timeout? > If a timeout occurred after the session is launched an extra action may be > required. Here is where I use "ICommandTimeoutPolicy". The default > implementation just terminates the session, but some debuggers can provide > their own implementation. I also fire 'ICommandTimedOutDMEvent' which may be > unnecessary, those implementations that need it can define their own timeout > policies but I thought it would be good if we provided it. I'm still not sure why not use the receiver of ICommandTimedOutDMEvent to do what the timeout policy does? I believe such a receiver can be made overrideable as easily as the ICommandTimeoutPolicy API. But maybe I'm wrong? > These are the ideas behind my implementation. I can just move the > implementation into 'GDBControl*' or even in 'AbstractMIControl' class, fix the > algorithm and other minor issues if you think the previous patch is too > complicated. I don't mind a complicated patch if you find it adds value. I just haven't quite grasped all that value yet :) Thanks for you patience with my questions.
(In reply to comment #37) > (In reply to comment #36) > > Marc, let me explain what I was trying to achieve in my previous patch. > > > > My intention was to implement the timeout functionality only for GDB based > > debuggers. Other backends may support the timeout internally and do not require > > an external implementation of it. > > I assume one of these backends is an MI based non-GDB debugger since there is a > > separation in DSF/GDB between GDB and MI. 'AbstractMIControl' seems to be one > > of the classes that have some non-GDB features. Is this correct? > > Originally, we kept classes that were for MI in one location while having > GDB-specific parts in other classes. This was because WindRiver had their own > backend that spoke MI. I don't believe that is an important use-case for them > anymore though. In fact, IIRC, Pawel mentioned that this division of classes > turned to be un-needed while complicating the code a lot. We'd have to check > with him and the list if someday we want to merge the classes - I'd like that a > lot as it would make the code simpler - although it would be a drastic API > change. Anyway, we're not at that point yet :) > > To make a long story short, officially, AbstractMIControl should not contain > GDB-specific code, as you very perceptively deduced. > I knew about WindRiver backend and suspected it had its own timeout implementation. That is the reason I don't want to implement the timeout feature inside AbstractMIControl class. But marking a command as failed can only be done in AbstractMIControl. That's why I added "commandFailed()" method to it. I am not very happy with it but can't see a better way of doing it. > > I agree, the timeout implementation in the previous patch is not a "real" > > service. But implementing it as a service provides the flexibility I am looking > > for. Anyone can replace or disable it without modifying other services. > > It is unclear to me why using a service makes it easier to replace instead of > using a method in GDBControl/GDBControl_7_0 (or another service) along the > lines of the pseudo-code: > > protected createCommandTimeout() { > return new GdbCommandTimeoutService(); > } > > In fact, for those that already override the GDBControl service, it will be > very easy to turn off the timeout. > I had the same discussion with Mohamed. Adding a new functionality to an existing service means that DSF/GDB extensions will automatically get this feature and have to disable it explicitly for their next releases. At the same time I assume that overriding the service factory is more common. Of course, this all is theory. The reality is much simple, nobody would even notice it until they need it. > Do you foresee changes in the CommandTimeout code that will later on require it > to be a service? Then it would make sense to be proactive. > > This being said, if you really feel the service solution is the best one, I'm > ok with your decision. > I am not 100% sure the service solution is right. Unless it is a generic timeout service which can be used to create a timeout for any object. > > > Another question is what do we want to do when a command is timed out. First of > > all we need to process the command as if an error occurred in GDB, i.e. mark > > the command as "timed out". If a timeout occurred when the launch sequence is > > executed the launch will be terminated. > > I like the idea of marking the command as "timed out" as it allows to cleanly > recover, if we ever choose not to terminate the entire session. > > What happens if the command does arrive, but after the timeout? > If the command is still in 'fRxCommands' list we get 'IllegalStateException' for calling RequestMonitor.done() twice :) Now I remove the command from the list and it the output string is processed by all registered 'IEventListeners' as an OOB record. Nothing happens because it doesn't have the right prefix. > > If a timeout occurred after the session is launched an extra action may be > > required. Here is where I use "ICommandTimeoutPolicy". The default > > implementation just terminates the session, but some debuggers can provide > > their own implementation. I also fire 'ICommandTimedOutDMEvent' which may be > > unnecessary, those implementations that need it can define their own timeout > > policies but I thought it would be good if we provided it. > > I'm still not sure why not use the receiver of ICommandTimedOutDMEvent to do > what the timeout policy does? I believe such a receiver can be made > overrideable as easily as the ICommandTimeoutPolicy API. But maybe I'm wrong? > I'll check if switching to use events is sufficient. Usually there is a time window between the occurrence of the event and when the listener receives it. This may cause some timing issues. > > These are the ideas behind my implementation. I can just move the > > implementation into 'GDBControl*' or even in 'AbstractMIControl' class, fix the > > algorithm and other minor issues if you think the previous patch is too > > complicated. > > I don't mind a complicated patch if you find it adds value. I just haven't > quite grasped all that value yet :) > > Thanks for you patience with my questions. I'll try to come up with a better (simpler) patch. I really like your attention to the details. That's how it is supposed to be and I have no problem with it. I am afraid it is rather the patience of my bosses that is being tested :)
(In reply to comment #30) > > I'm not sure if it would be possible to write some JUnit tests for this > > feature, but if it is, it would make it easier to test :) > > Actually, I have been thinking about it. I'm not 100% sure but think the > timeout service can be tested separately without running a session. This would > require to write a class that generates commands and register the service as a > listener. It may be hard to implement but I'll think about it. I just thought of something for unit tests. How about a normal debug session, where we send the command: 'target remote :<badport>' without a gdbserver listening on the other side? GDB will block for something like 10 seconds. Not only can we test the timeout feature having the 'target remote' command itself timeout, but we could send any other command while GDB is locked in the 'target remote' call; those commands won't get answered until 'target remote' is completed. That should allow to try some different combinations of command timeouts in eclipse. What do you think?
(In reply to comment #39) > (In reply to comment #30) > > > > I'm not sure if it would be possible to write some JUnit tests for this > > > feature, but if it is, it would make it easier to test :) > > > > Actually, I have been thinking about it. I'm not 100% sure but think the > > timeout service can be tested separately without running a session. This would > > require to write a class that generates commands and register the service as a > > listener. It may be hard to implement but I'll think about it. > > I just thought of something for unit tests. > How about a normal debug session, where we send the command: > 'target remote :<badport>' > without a gdbserver listening on the other side? > GDB will block for something like 10 seconds. > > Not only can we test the timeout feature having the 'target remote' command > itself timeout, but we could send any other command while GDB is locked in the > 'target remote' call; those commands won't get answered until 'target remote' > is completed. > > That should allow to try some different combinations of command timeouts in > eclipse. > > What do you think? Yes, that's possible. But I am more concerned about the algorithm. I would like to write a test that covers as many cases as possible. I hope this can be done without starting a gdb session.
Created attachment 213410 [details] Modified patch.
Marc, the new patch addresses most of the issues you mentioned in your comments. I just want to stress that the purpose of this patch is to catch the situations when GDB stops responding and handle it gracefully. The algorithm used in the patch is based on the assumption that GDB executes the commands sequentially. As you know DSF can send up to three commands to the queue which makes it difficult to find the exact time when the execution of a command is really started by GDB. I have made changes in the algorithm to make the time adjustments as close as possible. The patch also includes JUnit tests. There are two test cases. The first one sets the timeout value to minimal (1ms) and expects the session to timed out on the first command (-list-features). I am not quite sure about this test, it is not a "real life" case and I had problems with it originally, but it seems have fixed them. The second one is what you suggested I start a remote session without starting gdbserver. This case seems to work fine. Please, let me know what you think.
(In reply to comment #42) > Marc, the new patch addresses most of the issues you mentioned in your > comments. Thanks Mikhail! Since the patch is large, how would you feel about trying to use Gerrit for the review? I know it sounds intimidating, but after seeing some demos at EclipseCon, I think it is worth trying. I think the setup will take no more than 5 minutes. We can do the setup together if you want, as I'm still new to Gerrit too. In summary, you would add Gerrit as a remote git repo to your existing CDT local repo, and then you would push your changes to that new remote repo. That's it. After than, I login to gerrit and perform the code review, and we communicate comments and fixes through gerrit/git. Apparently, it should make the whole code review exchange much easier. What do you say?
(In reply to comment #43) > Thanks Mikhail! > > Since the patch is large, how would you feel about trying to use Gerrit for the > review? I know it sounds intimidating, but after seeing some demos at > EclipseCon, I think it is worth trying. > > I think the setup will take no more than 5 minutes. We can do the setup > together if you want, as I'm still new to Gerrit too. > > In summary, you would add Gerrit as a remote git repo to your existing CDT > local repo, and then you would push your changes to that new remote repo. > That's it. > > After than, I login to gerrit and perform the code review, and we communicate > comments and fixes through gerrit/git. Apparently, it should make the whole > code review exchange much easier. > > What do you say? I'll can try it. Can I do it from Eclipse? If so, what do I need to install?
(In reply to comment #44) > (In reply to comment #43) > I'll can try it. Can I do it from Eclipse? If so, what do I need to install? Yes, you can use EGit to do it (http://wiki.eclipse.org/Gerrit#Using_Gerrit_with_EGit). I'm having trouble accessing gerrit through the firewall right now. I'll try it from home tonight. Here is what I believe you need to do: 1- open 'git repositories' view 2- expand the repo that corresponds to the CDT repo 3- right-click on 'Remotes' and select "Create remote..." 4- give it a name like "gerrit_eclipse" and leave "configure push" selected 5- click on the 'Change...' button, next to the 'URI' text box 6- use the following URI: ssh://mkhodjai@git.eclipse.org:29418/cdt/org.eclipse.cdt.git 7- In the Ref mapping section, add a RefSpec specification of HEAD:refs/for/master Now your gerrit is setup. Whenever you want a code review, push the commits to this new remote.
(In reply to comment #45) > I'm having trouble accessing gerrit through the firewall right now. I'll try > it from home tonight. > > Here is what I believe you need to do: > > 1- open 'git repositories' view > 2- expand the repo that corresponds to the CDT repo > 3- right-click on 'Remotes' and select "Create remote..." > 4- give it a name like "gerrit_eclipse" and leave "configure push" selected > 5- click on the 'Change...' button, next to the 'URI' text box > 6- use the following URI: > ssh://mkhodjai@git.eclipse.org:29418/cdt/org.eclipse.cdt.git > 7- In the Ref mapping section, add a RefSpec specification of > HEAD:refs/for/master > > Now your gerrit is setup. > > Whenever you want a code review, push the commits to this new remote. Thanks Marc, I followed the instructions and have a new remote setup but haven't tried pushing anything yet. What am I supposed now, apply my patch to the local master branch and push to the Gerrit repository?
(In reply to comment #46) > Thanks Marc, I followed the instructions and have a new remote setup but > haven't tried pushing anything yet. What am I supposed now, apply my patch to > the local master branch and push to the Gerrit repository? Ok, I ran a test and everything worked as expected. Once you have setup your gerrit remote you should: 1- commit the code you want to review to a branch. That branch does not need to be the master branch, but it should have the latest code (rebased to master). 2- Right-click on the repo in the Git Repositories view and select "Push..." 3- Select the gerrit remote and press Next The following two steps you may have already done when you configured the remote gerrit: 4- select which branch you want to push. The easiest thing (and the one suggested in the Eclipse/Gerrit wiki) is to select HEAD as the 'from' branch. This means that whatever branch is currently checked out will be used for the push. 5- Select the destination branch. This is a special branch called 'refs/for/master'. The idea is that once the commit is 'approved' it will be merged to the master branch by gerrit. 6- complete the push. 7- go to the https://git.eclipse.org/r and you should see the change you just pushed (assuming you are watching the CDT project; if not, you can find it in the All section). Do you want to try that? I have to play around with my gerrit account setting to see if emails are configured properly, but once something is pushed to CDT, I believe I should get an email.
(In reply to comment #47) > (In reply to comment #46) > > > Thanks Marc, I followed the instructions and have a new remote setup but > > haven't tried pushing anything yet. What am I supposed now, apply my patch to > > the local master branch and push to the Gerrit repository? > > Ok, I ran a test and everything worked as expected. > > Once you have setup your gerrit remote you should: > > 1- commit the code you want to review to a branch. That branch does not need > to be the master branch, but it should have the latest code (rebased to > master). > 2- Right-click on the repo in the Git Repositories view and select "Push..." > 3- Select the gerrit remote and press Next > > The following two steps you may have already done when you configured the > remote gerrit: > > 4- select which branch you want to push. The easiest thing (and the one > suggested in the Eclipse/Gerrit wiki) is to select HEAD as the 'from' branch. > This means that whatever branch is currently checked out will be used for the > push. > 5- Select the destination branch. This is a special branch called > 'refs/for/master'. The idea is that once the commit is 'approved' it will be > merged to the master branch by gerrit. > > 6- complete the push. > 7- go to the https://git.eclipse.org/r and you should see the change you just > pushed (assuming you are watching the CDT project; if not, you can find it in > the All section). > > Do you want to try that? I am not sure that my ssh is set properly, otherwise I am ready to push the branch with the patch. I just don't want to leave my changes overnight in case I break something. Let's do it tomorrow so you can supervise the process.
For people's information, the review is ongoing through Gerrit with changeId 5523. https://git.eclipse.org/r/#/c/5523/ The new solution is much simpler than before and it makes me feel more confident about it. Thanks Mikhail! I have a couple more things to review, but I should be done soon.
(In reply to comment #35) Since we've moved to Gerrit to complete this review, some comments here may seem to have been left hanging, which is not the case. Here is what is happening: > > 1- ICommandTimeoutPolicy vs ICommandTimedOutDMEvent, could we just use the > event? Both removed, replaced with an ICommandTimeoutListener > 2- Should GdbCommandTimeoutService be a service or just a class? No longer a service. > 3- Just an idea: instead of adding the IGDBControlExtension API to know if the > CommandControl service has been completely initialized, we could have > DefaultGdbCommandTimeoutPolicy be a @DsfServiceEventHandler and listen for > DataModelInitializedEvent, that event would indicate that the initialization is > finished. I'm not saying it is a better way, but it would keep the change more > local to the timeout feature and avoid a new interface. No current need for a plublic interface to see if GDBControl is initialized. > 4- Do you want to support the usecase where the user only wants a timeout on > some commands, but all others have no timeout? In that case, the user would > need to set the default timeout to 0, while setting some custom timeouts. > Currently, the preference page does not allow to set the default timeout to 0. > If you want to support that case, there may be some changes needed in the > GdbCommandTimeoutService, where we may need to check for waitTimeout>0 in > calculateWaitTimeout() Does not seem to be of interest. > 5- I think the TimerThread algorithm does not cover every case. If I > understand correctly, it only checks the first command of the queue, although > any of the next commands may have a shorter timeout. This was wrong. GDB can only process one command at a time, so if GDB blocks on one command, we should only timeout on that command, which is what Mikhail has done. To illustrate, say the user sets the command timeouts to 10 seconds, except for -target-select which is set to 30 seconds. The user then connects to a target which takes 20 seconds to reply to -target-select, we wouldn't want to start timing out any command sent after -target-select, but instead, we need to start those command's timer once -target-select is done. > 6- AbstractMIControl only sends out 3 commands to GDB at a time, the other > commands are kept in the queue until we get an answer for at least one of the 3 > sent commands. Imagine this scenario: > a) long default timeout set > b) short timeout for register commands > c) the three sent commands are not getting an answer. Because of the long > timeout, there is no error happening yet > d) user opens the register view > e) register command are not sent out to GDB because 3 commands are pending => > short timeout is not triggered because the commands are not sent to GDB. This is also not a good idea. > 8-The new Advanced preference page is great. However we need to help the user > know how to specify the MI command. It was not clear to me how to specify it, > although now that I know, it is kind of obvious. How about an example at the > top of the preference page? "example: -thread-info" This was not addressed it seems. > 9- Can we make the trace logs more uniform? i.e., remove [] and add a prefix > such as [TMO], like AbstractMIControl.MI_TRACE_IDENTIFIER > Currently it looks like: > 326,383 [MI] 41-var-create --thread 1 --frame 0 - * um3 > [326,384] Command '-var-create - * um4' sent, timeout = 1000 Has been done.
(In reply to comment #50) > > 4- Do you want to support the usecase where the user only wants a timeout on > > some commands, but all others have no timeout? In that case, the user would > > need to set the default timeout to 0, while setting some custom timeouts. > > Currently, the preference page does not allow to set the default timeout to 0. > > If you want to support that case, there may be some changes needed in the > > GdbCommandTimeoutService, where we may need to check for waitTimeout>0 in > > calculateWaitTimeout() > > Does not seem to be of interest. > Actually this has been addressed. the dialog now allows setting the value of the default timeout to 0. > > 8-The new Advanced preference page is great. However we need to help the user > > know how to specify the MI command. It was not clear to me how to specify it, > > although now that I know, it is kind of obvious. How about an example at the > > top of the preference page? "example: -thread-info" > > This was not addressed it seems. > I have forgotten about this one. Will do.
(In reply to comment #51) > (In reply to comment #50) > > > 4- Do you want to support the usecase where the user only wants a timeout on > > > some commands, but all others have no timeout? In that case, the user would > > > need to set the default timeout to 0, while setting some custom timeouts. > > > Currently, the preference page does not allow to set the default timeout to 0. > > > If you want to support that case, there may be some changes needed in the > > > GdbCommandTimeoutService, where we may need to check for waitTimeout>0 in > > > calculateWaitTimeout() > > > > Does not seem to be of interest. > > > > Actually this has been addressed. the dialog now allows setting the value of > the default timeout to 0. Sorry, I hadn't noticed. But when I try it, it does not seem to work :) Try this: 1- set global timeout to 0 2- set custom timeout to (-target-select, 1000) 3- launch a manual remote session with no gdbserver 4- -target-select does not timeout after 1 second How about adding a JUnit test for that case?
(In reply to comment #52) > (In reply to comment #51) > > (In reply to comment #50) > > > > 4- Do you want to support the usecase where the user only wants a timeout on > > > > some commands, but all others have no timeout? In that case, the user would > > > > need to set the default timeout to 0, while setting some custom timeouts. > > > > Currently, the preference page does not allow to set the default timeout to 0. > > > > If you want to support that case, there may be some changes needed in the > > > > GdbCommandTimeoutService, where we may need to check for waitTimeout>0 in > > > > calculateWaitTimeout() > > > > > > Does not seem to be of interest. > > > > > > > Actually this has been addressed. the dialog now allows setting the value of > > the default timeout to 0. > > Sorry, I hadn't noticed. > > But when I try it, it does not seem to work :) > Try this: > 1- set global timeout to 0 > 2- set custom timeout to (-target-select, 1000) > 3- launch a manual remote session with no gdbserver > 4- -target-select does not timeout after 1 second > > How about adding a JUnit test for that case? I have to admit I didn't test this case assuming that it would just work. I'll fix it and add a test case for it.
(In reply to comment #53) > > But when I try it, it does not seem to work :) > > Try this: > > 1- set global timeout to 0 > > 2- set custom timeout to (-target-select, 1000) > > 3- launch a manual remote session with no gdbserver > > 4- -target-select does not timeout after 1 second > > > > How about adding a JUnit test for that case? > > I have to admit I didn't test this case assuming that it would just work. I'll > fix it and add a test case for it. Probably caused by: fTimerThread.setState((fTimerThread.getWaitTimeout() > 0) ? TimerThreadState.RUNNING : TimerThreadState.HALTED);
(In reply to comment #52) > But when I try it, it does not seem to work :) > Try this: > 1- set global timeout to 0 > 2- set custom timeout to (-target-select, 1000) > 3- launch a manual remote session with no gdbserver > 4- -target-select does not timeout after 1 second > > How about adding a JUnit test for that case? I added this comment to the Gerrit review.
Checked in. See Gerrit review https://git.eclipse.org/r/#/c/5523/.
