Re: [cdt-dev] Timeouts for GDB/MI commands (Was: [DSF] Making Sequence m

[Marc Khouzam <marc.khouzam@xxxxxxxxxxxx>]

> -----Original Message-----
> From: Vladimir Prus [mailto:vladimir@xxxxxxxxxxxxxxxx] 
> Sent: Monday, October 24, 2011 10:43 AM
> To: cdt-dev@xxxxxxxxxxx
> Cc: Marc Khouzam; Pawel Piech; Mikhail Khodjaiants
> Subject: Re: [cdt-dev] Timeouts for GDB/MI commands (Was: 
> [DSF] Making Sequence more robust)
> 
> On Monday, October 24, 2011 17:56:03 Marc Khouzam wrote:
> > > -----Original Message-----
> > > From: cdt-dev-bounces@xxxxxxxxxxx
> > > [mailto:cdt-dev-bounces@xxxxxxxxxxx] On Behalf Of Vladimir Prus
> > > Sent: Saturday, October 22, 2011 5:55 AM
> > > To: Pawel Piech; Mikhail Khodjaiants
> > > Cc: cdt-dev@xxxxxxxxxxx
> > > Subject: [cdt-dev] Timeouts for GDB/MI commands (Was: [DSF]
> > > Making Sequence more robust)
> > 
> > Hi,
> > 
> > I think it would be great to add some timer infrastructure
> > for commands sent to GDB.  Thanks for pushing this forward.
> > Please see below for some comments.
> > 
> > > On Thursday, October 20, 2011 21:40:17 Pawel Piech wrote:
> > > > > 2. While a general repository for outstanding RM's might
> > > 
> > > be a good idea,
> > > 
> > > > > what would you say if we start small, and implement the
> > > 
> > > solution that
> > > 
> > > > > would remember outstanding RequestMonitor instances for
> > > 
> > > some limited
> > > 
> > > > > cases (for example, 'target *-remote' commands in
> > > 
> > > DSF-GDB, and then
> > > 
> > > > > cancel those RequestMonitors after a timeout? This is somewhat
> > > > > orthogonal from the initial goal of catching lost 
> RMs, but seems
> > > > > sufficiently related and immediately useful.
> > > > 
> > > > This is something that could be done at the service level,
> > > 
> > > where these
> > > 
> > > > commands are issued.  If we expect that the back-end may
> > > 
> > > never complete
> > > 
> > > > some commands then in those cases a timeout guard should even be
> > > > required.  For example, we queue commands with the command
> > > 
> > > control, but
> > > 
> > > > we count on the process monitor to notify us if the back
> > > 
> > > end process
> > > 
> > > > dies.  At that point we complete any outstanding command in
> > > 
> > > queue with
> > > 
> > > > an error.  So our process monitor is the guard.
> > > 
> > > We discussed this approach internally (turned out to be
> > > faster), and Mikhail has proposed approach like this:
> > > 
> > > 1. Add new interface:
> > >     interface IMIControlTimeoutPolicy {
> > >     
> > >         void commandSend(CommandHandle);
> > >         void commandDone(CommandHandle);
> > >     
> > >     }
> > 
> > ICommandListener has commandSent() and commandDone().
> > How about using 'sendCommand' and 'processCommandReply' to avoid
> > confusion?
> 
> Well, 'sendCommand' sounds like a methods that sends a command,
> as opposed to a method that is notified that a command is sent.
> Maybe, createTimeoutPolicy should just return ICommandListener?

So I mis-understood what you suggest.  When you say that TxThread/RxThread
would call these methods, I thought it was to send and receive
the commands in a way that could be overridden.

Instead, I gather you want to be able to do extra work when a
command is sent, so as to set a timer, right?
In that case, maybe you simply want to use the existing:
  ICommandControl.addCommandListener(ICommandListener)
then add a timer in there?


> > > 2. Add new method AbstractMIControl.createTimeoutPolicy
> > > (returning null by default).
> > > Adjust the code so that when this method returns non-null,
> > > the commandSend and commandDone
> > > methods of the result are called by TxThread/RxThread as 
> appropriate.
> > 
> > Sure, that mostly makes that code easy to override.
> > The interesting part will be the code that handles the timers.
> > 
> > > 3. Make GDBControl.createTimeoutPolicy return a policy that
> > > sets a timer whenever a command
> > > is sent, and if the command is not done within a timeout,
> > > just calls GDBControl.shutdown.
> > 
> > I wonder if such a solution is sufficient?
> > Do we want to recover for a missing reply, or should it never
> > happen and we can indeed just abort?
> > 
> > A more flexible solution, which may be overkill as it may never
> > be triggered would be to allow for different behaviors
> > for different MI commands and different timeouts as well
> > (I think CDI has that already).
> > 
> > How about adding some methods to MICommand?  Something like:
> >   getTimeout() // Returns timeout value for this MICommand
> >   timeout(RequestMonitor)    // Called if the timer expires before
> >                              // a reply has been received.  
> This method
> >                              // will decide what to do with the RM
> > 
> > We'd put in default implementation in MICommand, but it 
> would be possible
> > to override them in individual commands, such as 'target *-remote'.
> > 
> > I'm imagining that the MICommand.timeout(RM) will deal with the RM
> > so that the service that sent the command can decided what to do
> > in that case.
> > 
> > But again, this may be overkill if we don't expect this to happen
> > often or at all.
> > 
> > Are you trying to address a specific scenario or a general safety
> > mechanism?
> 
> We care both for initial connection timeout (-target-select) 
> and a case
> where target goes away in the middle of debug session (wedged board).
> We actually discussed individual handling for commands on 
> internal IRC,
> and Mikhail suggested this is not a good idea, since it was 
> implemented
> this way in CDI and just caused multiple error messages in case target
> is gone. Obviously, trying to implement error recovery for 
> all commands
> is a major undertaking in all cases.

If it wasn't so useful for CDI, let's not do it just yet for DSF :-)

> So, I would suggest not to complicate interface until we sure 
> per-command
> timeout policy is both useful and implementable.
> 
> What do you think?

Ok with me.

Marc

> 
> -- 
> Vladimir Prus
> CodeSourcery / Mentor Graphics
> +7 (812) 677-68-40
>