BIND 10 #352: Improve Xfrout request handling strategy

[BIND 10 Development]

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

  * owner:  zhanglikun => jinmei


Comment:

 Replying to [comment:9 jinmei]:
 > First off, if I understand it correctly, the change to the mixin
 requires changes to other part of the code (e.g. xfrout), but they've not
 been modified.
 >

 After some hint from your review result, I changed the serve_forever() and
 shutdown() logic back. see r3298. so now we don't need to touch of the
 code of xfrout/cmdctl. Also, I realized the best way for avoid race
 condition is: avoid use the variable that's cause the problem.


 > Secondly, while I re-read the documentation of the standard socketserver
 library, I noticed serve_forever() isn't expected to be overridden.
 >
 > {{{
 > class TCPServer(BaseServer):
 >     ...
 >     Methods for the caller:
 >     ...
 >     - serve_forever(poll_interval=0.5)
 >     ...
 >     Methods that may be overridden:
 >     ...
 > }}}
 >
 > Doesn't this mean this mixin implementation is somehow in a gray zone?
 That is, even if it may work with the current socketserver implementation
 it may not be guaranteed for future versions?

 Yes, the documentation say serve_forever() and shutdown() are not expected
 to be overridden. But also there is some TODO in the
 socketserver.serve_forever(): see
 {{{
             # XXX: Consider using another file descriptor or
             # connecting to the socket to wake this up instead of
             # polling. Polling reduces our responsiveness to a
             # shutdown request and wastes cpu at all other times.
 }}}

 so we have to do it by ourselves before python fix the problem, and again,
 once python fix it, we can just drop this mixin.

 >
 >
 > I've noticed this is a repeated style issue in diffs committed by CNNIC
 developers.  Perhaps there's some Chinese character-specific setting in
 your editor or IDE that tends to this insert the spaces at EOL?  You might
 want to check your local configuration.
 >
 > (My editor automatically removes such redundant spaces when I continue
 to a next line, so it's rare for me to see this type of style issue even
 if I don't carefully type)
 >

 Thanks for your fix, and the spaces at EOL was left when I edited the
 function description, some spaces between two words was not removed.


 > It's still not sufficient to me.  IMO it still doesn't explain "why and
 when" sufficiently.  It also doesn't explain the usage of the caller side
 (according to the newest test, it seems the caller is assumed to call
 server.serve_forever().  it's not obvious).  It doesn't explain shutdown()
 cannot be called from the handler, which is internally invoked in a
 separate thread, because it would cause a dead lock (if I understand it
 correctly).  the socketserver documentation describes that.  Again, we
 need to provide the same level of details.  This also seems to be another
 evidence that we are not so experienced with thread programming.
 >
 > There's one other minor editorial issue: why did you change
 socketserver.TCPServer to socketserver::TCPServer?  It seems to me the
 former is correct for python.
 >


 why and when ==
 {{
     Mix-In class to override the function serve_forever()
     and shutdown() in class socketserver::TCPServer.
       serve_forever() in socketserver.TCPServer use polling
     for checking request, which reduces the responsiveness to
     the shutdown request and wastes cpu at all other times.
 }}

 I can't find any better words to describe this mixin, I would appreciate
 your better words for 'why and when' if you can give. thanks.


 > Fixing a clear bug like a race is a bottom line, but the more
 fundamental question is whether self.!__serving is necessary in the first
 place.  I still don't see why we need it.  A mutable variable is always a
 source of troubles, so if we don't need one we should eliminate it.


 yes, this is the hint for the latest revision r3298


 > I'm not sure if continue is the correct behavior.  I actually don't know
 what kind of error other than EINTR is expected in this context, but if
 it's a permanent error (e.g. due to some broken state in the socket), the
 error will repeat and this loop continues infinitely (until we get a
 shutdown message).  We should clarify which errors we can receive here,
 and continue only for transient errors (which may be none or all).


 I also don't know what kind of error other than EINTR will happen, but
 this time I change the code as print the error and break. Not sure it's
 the best way.

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