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             |  
-------------------------------+--------------------------------------------

Comment(by jinmei):

 Replying to [comment:6 jelte]:
 > Ok, i think it should be about ready now;

 > I also added timeouts to the python version, though in this case no
 exception is raised (but the answer and envelope are None). Should we have
 an exception here too (ie if (blocking and timeout and no answer) raise
 TimeoutError)?
 >
 I wondered this point when I first saw the code (without checking this
 comment closely).  It would depend on whether we want to separate timeouts
 from other cases where an empty answer is returned.  I'm not so sure which
 is the case in this scenario, but in general I guess we want to know it's
 a timeout if it is.

 '''One substantial point in the patch, if I understand it correctly:'''

 In session.cc, it seems possible that SessionImpl::setResult can be called
 after SessionImpl::readData() returns.  If that happens invalid stack
 positions will be overritten.

 One easy way to avoid that scenario is to make xxx_result and xxx_code
 class member variables.  There's still a risk that setResult() could be
 called after the !SessionImpl object is destructed, but I guess it's not
 specific to the session class and can be considered a separate issue
 (including whether we need to address it).

 Other comments (below) are basically minor.

 '''common'''
  - What's the rationale of the magic number of 4 seconds?  Please
 describe.  I'd also avoid hardcoding the magic number in the code logic:
 I'd define a constant variable with the rationale description and use that
 variable in other part of the code.  I'd also centralize the definition of
 the default timeout value for python and C++, if it can be done easily.
  - I wish we could test the case where we specify 'no timeout', but I
 understand it's very difficult if not impossible.

 '''session.cc'''
  - SessionImpl::setTimeout()/getTimeout() don't have to be (and so
 shouldn't even be) virtual

 '''session.h'''
  - I'd make (Abstract)Session::getTimeout() const member function.
  - I'd move the generacl description of set/getTimeout() to the abstract
 base class.  Also, the description of "defaults to 4000" for setTimeout()
 doesn't seem to be very correct (or it seems to be ambiguous) in that 4000
 is not the default parameter of this function.  I'd add a description to
 the (concrete) Session about the timeout behavior, and explain the default
 timeout value there.  See also the comment about hardcoding the magic
 number (above).
  - according the latest yet-undocumented coding guideline there should be
 a blank line before "\brief"
  - I've noticed some minor editorial/coding style issues and directly made
 suggested changes to the branch (r2696).

 '''session_unittest.cc'''
  - why explicitly specifying the global namespace for unlink?  It's not
 incorrect, but not consistent with how we specify standard C libraries in
 other part of the code...hmm, maybe it's because it conflicts with some
 name in the asio name space?
  - it's a matter of taste, but I'd use a fixture (TEST_F) for tests that
 have a common setup phase (like unlinking the file) to avoid having
 duplicate setup code.
  - it's not actually specific to this patch, but I'd avod using int (or
 size_t in session.cc), e.g.:
 {{{
         const unsigned int length_net = htonl(length);
 }}}
   in fact, in this specific case it's actually a big buggy because the
 size of 'int' may be smaller than 32 bits.  We should use uint32_t,
 uint16_t, etc. (but this can go to a separate ticket).
  - the in-function block in connect_ok_connection_reset is ugly and
 obscures the intention (even with the comment).  I'd add a close() method
 to !TestDomainSocket and call it explicitly.

 '''msgq.py.in'''
  - is it related to the timeout hack, or an independent change?  if so,
 maybe some comments may help.

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