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:7 jinmei]:
 > Replying to [comment:6 zhanglikun]:
 > > 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, :)

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

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

 >  - I still don't see why we need to give a specific default value (i.e.
 0.5) to poll_interval.  since it's not used, I'd specify None.

 Already done as you suggested in the latest revision.

 >  - this operation seems to be redundant:
 > {{{
 >         self.__is_shut_down.clear()
 > }}}
 >   because at this point !__is_shut_down should always be cleared.

 Good suggestion. :) thanks

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

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

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