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

[BIND 10 Development]

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

  * owner:  jelte => jinmei


Comment:

 Replying to [comment:13 jinmei]:
 > '''About r2699'''
 >  - I'd avoid using a macro.  I'd define it as either a namespace or
 class constant, e.g.:
 > {{{
 > class SessionImpl {
 > private:
 >     static const size_t MSGQ_DEFAULT_TIMEOUT = 4000;
 > ...
 > };
 > }}}

 ack

 >  - isn't it possible to move setup for 'sess' to the fixture?
 > {{{
 >     Session sess(my_io_service);
 > }}}
 >

 yes, it 'feels' a bit weird though, since in essence we're now setting up
 the session before the 'host' testdomainsocket. But since establish() is
 called manually anyway this shouldn't be a problem. Updated.

 > '''About r2716'''
 >  - I have some suggested editorial changes (committed to the branch)

 thanks

 >  - setResult() should better be in an anonymous namespace.

 done

 >  - I'd remove the hardcoded value from:
 > {{{
 >     // timeout for blocking reads (in seconds, defaults to 4000)
 > }}}

 done

 >  - is it okay to do io_service().reset()?  According to the ASIO
 reference "this function must not be called while there are any unfinished
 calls".  I'm afraid we cannot assume such a thing in general.

 thing is, we can't assume we don't need it either; if io_service has
 decided it ran out of work, it'll stop doing anything on run_one(), and
 our while loop will run forever (this happens in one of the test cases
 here). The documentation also says that you need to call it before any
 second or later set of calls to run() or run_one().

 >  - I don't understand why we need to check getTimeout() != 0 here:
 > {{{
 >             if (read_result && getTimeout() != 0) {
 > }}}

 If timeout is set to 0, the timer isn't started, and waiting for it to end
 would result in an eternal loop (after data has arrived). Added comment.

 >  - The following line isn't necessarily incorrect but looks a bit
 awkward:
 > {{{
 >         if (read_result->value() != 0) {
 > }}}
 >   why not this?  This seems to be more consistent with the other usage
 of asio::error_code in this code:
 > {{{
 >         if (*read_result) {
 > }}}

 Personal preference; I don't have a strong opinion, but it is more clear
 to me as a reader what we are checking for here (success result). Unless
 i'm wrong in reading the code, there's no asio::error_code::success or
 something similar, so this would have to do. If you think *read_result
 directly is more clear, we can change it.

 >
 > Other changes look okay.

 Ok, changes committed in r2735

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