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

Comment(by 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.

 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?

 > > > The lastest change has been committed, see r3149, please have a
 review. thanks.
 > >
 > > First, there are some redundant white spaces after EOLs.  I've removed
 them in r3167.
 >
 > Thanks for your fix, :)
 >
 The latest revision introduced additional redundant white spaces.  Fixed
 in r3287.  I also fixed a minor typo in r3289.

 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)

 > > Second, now that we've seen real race conditions (Trac #335), I've re-
 reviewed the entire code more carefully, and have had additional concerns.
 Revised review comments with the new issues follow:
 >
 > The race condition problem should has been fixed in r3270, please have a
 look.
 >
 The race seems to be fixed.  But I still see some substantial issues with
 the implementation.  See below.

 > >  - I don't think documentation is sufficient yet.  it still doesn't
 talk about why and when we need this mixin; it doesn't explain the seeming
 assumption of using threads (in the description of the ServeMixIn class).
 I suggest you refer to socketserver.py and provide that level of details.
 >
 > Update some documentation. the mixin doesn't assumpt of using threads.
 >
 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.

 > >  - do we need self.!__serving?  in which case do we need it even if we
 already have synchronization mechanisms?  note also that accessing
 self.!__serving isn't protected and can cause a race condition (consider,
 e.g. the case shutdown() is called immediately after invoking the thread,
 then serve_forever() is called from the thread.  !__serving will never be
 False.).
 >
 > race condition should has been fixed in the revision.
 >
 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.

 > >  - I'm afraid this raise is dangerous:
 > > {{{
 > >             except select.error as err:
 > >                 if err.args[0] != EINTR:
 > >                     raise
 > > }}}
 > >   consider the case where shutdown is called and the parent thread is
 waiting on !__is_shut_down.  what if the above raise is triggered under
 this condition?  doesn't it cause a deadlock?
 >
 > the raise has been removed, if the exception select.error, just
 continue.
 >
 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).

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