BIND 10 #216: Xfrin: Implement the feature items in TODO file.

[BIND 10 Development]

#216: Xfrin: Implement the feature items in TODO file.
------------------------------+---------------------------------------------
      Reporter:  zhanglikun   |        Owner:  jinmei              
          Type:  enhancement  |       Status:  reviewing           
      Priority:  major        |    Milestone:  y2 6 month milestone
     Component:  xfrin        |   Resolution:                      
      Keywords:               |    Sensitive:  0                   
Estimatedhours:  0.0          |        Hours:  0                   
      Billable:  1            |   Totalhours:  5.0                 
      Internal:  0            |  
------------------------------+---------------------------------------------

Comment(by jinmei):

 Replying to [comment:35 shentingting]:

 > > > ponit3: I changed select function, now it can select the socket
 which is writable or readable.
 > > >
 > > Quick check: why do we need to check writability?
 > > (intentionally leaving the owner being me)
 > because select is asyncronous, so I set the socket non-blocking. The
 connect function is non-blocking, it will return immediately when we call
 it, this will generate EINPROCESS exception, we will ignore this and use
 select to tell us whether it is really readable or writable.

 I've given a more complete review to the latest change, and, I regret to
 say this but I'm not so happy about it.

 But before discussing that point, I'd like to check this point: are you
 sure you addressed (including rejecting some possibly) all points?  From a
 quick check this one is still open:

  - I guess !MockXfrinConnection.connect_to_master() should return True.

 Admittedly this is a minor point, but since I've not seen any response on
 the comment or code change, I cannot be sure if you even did any decision
 about this, and this one also makes me wonder there may be other remaining
 issues.

 Now, about the latest change.  I guess you tried to address this comment:

  - I believe we should use select for connect and send while watching a
 shutdown event. consider, e.g., the case where a shutdown event is sent
 when the primary server is dead and connect blocks. but it's okay for me
 to leave this enhancement to another ticket (or we should rather do so)

 I appreciate the effort, but the code doesn't solve the real issue: "while
 watching a shutdown event".  This code essentially does blocking
 connect/write by doing select() only for the writability followed by the
 corresponding write operation.  Also, scattering select() calls over the
 source code decreases readability.

 As I commented before, I suspect this will be a sufficiently large
 changeset and we should defer it to a new ticket, especially because we've
 been spending quite a long time for this ticket already.  May I
 respectfully suggest we cancel the changes for non-blocking connect/send?

 Some other comments on specific parts of the code:

  - why do we need to cover EALREADY?
 {{{
 +            if why.args[0] in (EINPROGRESS, EALREADY, EWOULDBLOCK):
 +                return
 }}}
   As far as I can see we shouldn't see this error in our scenario.
  - why was the "It's extracted..." comment removed?
 {{{
          This method is a trivial asynchronous I/O routine using select.
 -        It's extracted from _get_request_response so that we can test the
 -        rest of the code without involving actual communication with a
 remote
 -        server.'''
 +        It checks which socket is writable or readable.'''
 }}}
   This part still applies with the latest change, and IMO this type of
 "why we do this" is one of the most important comments.  "What we do" type
 of comment like "It checks which..." may also help, but (IMO) is less
 important than "why" because, especially in such a trivial code, "what" is
 often obvious (for that matter, "writable or readable" is not really
 correct, because, at least according to the method interface, we also
 catch error events).

 p.s. you don't have to feel sorry for anything.  what I've been asking is
 basically "my rule", rather than agreed review protocol of the team.  You
 may also have your own rule about how to handle review comments, and
 that's probably just different from mine.  I've been asking what I've
 asked based on my rule, but you may think it's not reasonable.  If so,
 please feel free to make an objection.  We can then discuss where we can
 make a compromise.

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