BIND 10 #299: AXFR fails half the time

[BIND 10 Development]

#299: AXFR fails half the time
-----------------------------+----------------------------------------------
      Reporter:  zzchen_pku  |        Owner:  zzchen_pku
          Type:  defect      |       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 jinmei):

  * owner:  jinmei => zzchen_pku


Comment:

 Replying to [comment:35 zzchen_pku]:

 > >   when shutdown happens, shouldn't the !XfroutSession be able to
 detect that via the _shutdown_sock?
 > I remember vorner mentioned before:
 >
 [snip]
 > Just to ensure the real shutdown comes.
 >
 Ah, okay.  But the intent is way too difficult to understand just from the
 code.  Please add comments about why we need to do that way.

 > >  - when EINTR happens, recv_fd() will be called even if it may not be
 readable.
 >
 With the new code, when errno.EINTR is received the while loop is
 terminated.  Is that the intent?  Also, this notation is not readable to
 me:

 {{{
                 if not e.args[0] == errno.EINTR:
 }}}

 I'd say either
 {{{
                 if e.args[0] is not errno.EINTR:
 }}}
 or just use the plain old style
 {{{
                 if e.args[0] != errno.EINTR:
 }}}
 which IMO is even more readable than "not A == B".

 > >  - (this is not specific to this patch but anyway) I'm not sure if
 it's guaranteed all data is received here:
 > > {{{
 > >             msgdata = self.request.recv(msg_len)
 > > }}}
 > >   isn't it possible (at least in theory) that it's a partial read?
 > The xfrout request message shouldn't be too large, but in order to avoid
 partial read, have updated the recv logic.

 > > '''fd_shar_python'''
 > > {{{
 > > static PyObject *XFR_FD_REV_FAIL;
 > > }}}
 > >   as far as remember this usage is actually even "deprecated".
 (although this type of cleanup may better be deferred to a different
 ticket)
 > >  - also please add documentation of this constatnt (at least in C++,
 and if possible in the python wrapper)
 >
 As for doc, please make it in the doxygen style.

 > >  - error cases should be handled properly here:
 > > {{{
 > >     XFR_FD_REV_FAIL = Py_BuildValue("i", isc::xfr::XFR_FD_REV_FAIL);
 > >     PyModule_AddObject(mod, "XFR_FD_REV_FAIL", XFR_FD_REV_FAIL);
 > > }}}
 > Updated.
 >

 Mostly okay, but there's one minor style issues:
 {{{
 PyObject *XFR_FD_RECEIVE_FAIL ...
 }}}
 should be
 {{{
 PyObject* XFR_FD_RECEIVE_FAIL ...
 }}}

 Finally, a new issue with the latest change in xfrout_test.py.  The
 following comment is not true any more now that it also overrides
 _send_data():

 {{{
 # We subclass the Session class we're testing here, only
 # to override the __init__() method, which wants a socket,
 class MyXfroutSession(XfroutSession):
 }}}

 and, in any event, this comment seems to be incomplete (ending with a
 comma).

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