BIND 10 #352: Improve Xfrout request handling strategy

[BIND 10 Development]

#352: Improve Xfrout request handling strategy
------------------------------+---------------------------------------------
      Reporter:  zzchen_pku   |        Owner:  zzchen_pku
          Type:  enhancement  |       Status:  reviewing 
      Priority:  major        |    Milestone:            
     Component:  xfrout       |   Resolution:            
      Keywords:               |    Sensitive:  0         
Estimatedhours:  0.0          |        Hours:  0         
      Billable:  1            |   Totalhours:  0         
      Internal:  0            |  
------------------------------+---------------------------------------------
Changes (by jinmei):

  * owner:  jinmei => zzchen_pku


Comment:

 Replying to [ticket:352 zzchen_pku]:
 > Xfrout server polls for shutdown every 0.1 seconds. Polling reduces
 Xfrout server's responsiveness to a shutdown request and wastes cpu at all
 other times. Consider using interrupt-driven instead of polling.

 The idea looks good, but I have some possibly substantial comments about
 the implementation.

 '''serve_mixin.py'''
  - I think we need more documentation about this class.  there seem to be
 many hidden assumptions, which are not clear from the code itself and the
 current documentation.  For example, it seems this class is assumed to be
 used with socketserver.ThreadingMixIn, but it's not clear.  It's also not
 clear whether we expect to have multiple (derived) instances of this mixin
 class.  Also non obvious is what we'd expect with poll_interval, which is
 unused.  Overall, we need to descrive the intention of the class, expected
 usage, and other non trivial implementation decisions.
  - Related to the point of whether we have multiple instances, is it okay
 to make _is_shut_down and _read/write_sock class variables?  If I
 understana it correctly they will be shared by all instances, and I
 suspect it would cause undesirable effects.
  - we may want to declare ServeMixIn._serving as "private" more clearly by
 renaming it to __serving
  - Related to the documentation issue, just from the code the purpose of
 the poll_interval of serve_forever() isn't clear because it's unused.  we
 need some note in comments about this point.  Also, I'm not sure whether
 we need to bother to provide the default parameter.
  - There's inconsistency between comment and code:
 {{{
             # block until the self.socket or self._read_sock is readable
             try:
                 r, w, e = select.select([self, self._read_sock], [], [])
 }}}
   comment says self.socket whle the code uses just self.  Also, while not
 incorrect, having "self" (which is a socketserver object) and "_read_sock"
 (which is a socket object) in a list seems a bit awkward.
  - I'd add a sanity check for the received data from the socket pair,
 i.e., check to see if the received data is 'anydata'.  (and I'd define a
 constant variable to minimize hardcoding the magic term)

 '''serve_mixin_test.py'''
  - I suggest replace 'localhost' with 127.0.0.1.  the former may require
 transaction with an external DNS server, which may block and/or fail.

 '''utils/tests/Makefile.am'''
  - there's a typo that makes the test fail.  I've fixed it in the branch
 (r3138)

 '''!ChangeLog'''
  - is the "author" (=zhang likun) correct?

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