BIND 10 #296: Timeouts on msgq's cc-channel

[BIND 10 Development]

#296: Timeouts on msgq's cc-channel
-------------------------------+--------------------------------------------
      Reporter:  jelte         |        Owner:  jelte               
          Type:  enhancement   |       Status:  new                 
      Priority:  major         |    Milestone:  y2 6 month milestone
     Component:  Unclassified  |   Resolution:                      
      Keywords:                |    Sensitive:  0                   
Estimatedhours:  0.0           |        Hours:  0                   
      Billable:  1             |   Totalhours:  0                   
      Internal:  0             |  
-------------------------------+--------------------------------------------

Comment(by jinmei):

 Not sure if we are now in the formal review cycle:-)  Would you like it?

 If this is a formal review I'd request usual check list:
  - add test please
  - provide changelog entry please
  - and perhaps more

 Replying to [comment:3 jelte]:

 > >  - overall it seems to work as intended, but I'd confirm it works even
 if there's another outstanding event and it's completed before this
 async_read() is done.
 >
 > and also block until such is the case?

 Not fully thought about what should be specifically tested, but yes,
 probably.

 > I think it'll require quite a bit of redesign of the msgq interface, as
 well as the way it is used (which i suspect is the reason parts of it now
 do an 'empty' async read followed by a sync read if that one is triggers)
 >
 > But yes, we should consider it.

 A middle term solution might be to extend our ASIO wrapper to support this
 mode and have other modules use it.  In any case, that's beyond the scope
 of this ticket.

 > >  - is it okay to ignore other errors than operation_aborted in
 async_read()?  it's also not clear what this code tries to do by
 explicitly excluding operation_aborted.
 >
 > No the 'real' errors aren't handled right now; the operation_aborted is
 an 'error' value that is given when cancel() is called, in which case the
 'i am done' boolean isn't set here. Perhaps we should not use a bool but a
 return code directly.

 The 'real' errors don't seem to be handled anywhere...and yes, we should
 probably pass the error code itself back to the caller.

 Also, as commented this code logic is not so trivial.  Some comments will
 help.

 > >  - I thought the type of the error_code arg in asio callbacks is const
 asio::error_code&.

 It still seems to lack '&'.

 And one additional comment:

  - personally, I'd avoid heavily relying on the default parameter.  since
 it's currently used in a (essentially) private function to session.cc and
 all usage uses the default value, I'd rather define a class constant
 SessionImpl and use that value in readData().  Please also describe the
 rationale (if any) of the magic number of 5.

-- 
Ticket URL: <http://bind10.isc.org/ticket/296#comment:4>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development