BIND 10 #1454: Pass UPDATE packets from b10-auth to DDNS module

[BIND 10 Development]

#1454: Pass UPDATE packets from b10-auth to DDNS module
-------------------------------------+-------------------------------------
                   Reporter:  jelte  |                 Owner:  jinmei
                       Type:  task   |                Status:  reviewing
                   Priority:  minor  |             Milestone:
                  Component:  DDNS   |  Sprint-20120124
                   Keywords:         |            Resolution:
            Defect Severity:  N/A    |             Sensitive:  0
Feature Depending on Ticket:  DDNS   |           Sub-Project:  DNS
        Add Hours to Ticket:  0      |  Estimated Difficulty:  3
                  Internal?:  0      |           Total Hours:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:14 vorner]:

 > > accept() involves the creation of SocketSessionReceiver, which could
 > > fail and result in an exception.  [...]
 >
 > Hmm, right, there might be some that would be recoverable. But I don't
 really think these would happen in practice enough to get worried about
 them (eg. ECONNABORTED could happen, but auth does not give up connecting
 now).

 In my experience, any things "that wouldn't happen in practice" will
 happen in real production systems and will surprise us.  As for
 ECONNABORTED I can imagine an instance of b10-auth could crash
 immediately after trying to establish a connection.  But from what you
 said the main difference in our views doesn't seem to be how likely
 the specific exceptions can happen, but whether we'd expect having
 recoverable exceptions in this context in general.  I thought we
 would, but you rather seem to assume all non recoverable errors should
 be handled within the underlying subroutines (right now they are
 check_command(), accept(), handle_session(), there will be more as we
 extend the module).  I'm not so sure if that's reasonable (for
 example, this assumes check_command should only raise fatal
 exceptions, but we cannot fully control the behavior in terms of
 modularity), but I see that's one approach.  In that case, however, we
 should revise the comment at the beginning of the while loop: we need
 to clarify the assumption on the subroutines.

 > > So, as a compromise it may be okay for now to let it die in that case,
 > > but I'd at least explicitly log that event within the loop before
 > > doing so.
 >
 > There's bunch of log-exception calls in the main function (down in the
 file, outside of the class). I think these could be enough.

 I suspect this is another form of difference on the view point.  I
 thought we'd expect various different types of exceptions that we'd
 like to catch here but will let them be propagated as a compromise.
 So they would be caught in the generic "Exception" block at the top
 level, which wouldn't be helpful very much because it's too generic.

 If, on the other hand, we really assume there would only be fatal
 exceptions, it makes sense to simply pass them through to the top
 level where they are logged.

 So, in summary, my revised suggestion is to match the comment and the
 code assumption:

 - If we really expect not to see non fatal exceptions in the while
   loop, revise the comment accordingly and add note that the
   subroutines shouldn't propagate recoverable errors as exceptions
 OR
 - If we propagate exceptions (that may or may not be fatal) as a
   compromise (as commented), catch them in the while loop and log that
   we catch them and will let the process die as a compromise.

 > > - This comment doesn't seem to be addressed (even if only to reject
 > >   it): I'd move logging after close() so that even if logging raises
 > >   an exception it at least completes the cleanup:
 >
 > I probably moved a different line previously. Anyway, this is not so
 critical with python, because it would lose the reference and it'd close
 it automatically.

 But in the original code self._socksession_receivers[fileno] would
 stay.  Anyway I saw the code was updated so I have no problem with the
 latest code.

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