BIND 10 #1429: Commands for sockets

[BIND 10 Development]

#1429: Commands for sockets
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  vorner                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20111220
                  Component:  Boss   |            Resolution:
  of BIND                            |             Sensitive:  0
                   Keywords:         |           Sub-Project:  Core
            Defect Severity:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:         |           Total Hours:  0
  Socket creator                     |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:9 vorner]:

 > > - socket_request_handler
 > >  - Why is logging still TODO?  Same for socket_consumer_dead()
 >
 > It was caused by inherently unreliable human memory. It should be fixed
 now.

 - BIND10_LOST_SOCKET_CONSUMER doesn't include the parameter in the
   message description, resulting in:
 {{{
 BIND10_LOST_SOCKET_CONSUMER consumer of sockets disconnected, considering
 all its sockets closed @@Missing placeholder %1 for '42'@@
 }}}
   (If you normally don't do this) I suggest trying to have the program
   print newly added log message by 'B10_LOGGER_DESTINATION=stdout make
   check | grep <msg_id>'.  This is my usual practice when I add a new
   log ID.
 - Also about BIND10_LOST_SOCKET_CONSUMER: I'd not categorize it as an
   "error".  Even though it may be somewhat unexpected if the remote
   process is abruptly terminated, it can happen and in the boss's
   point of view in this context it's an expected event that the
   connection is reset in this case.  So I'd suggest using INFO in this
   case.
 - For a similar reason I'm not sure if "error" is good for
   BIND10_NO_SOCKET, although this case is mooter because it might be
   an internal error, too.  I'd still think info is better if the "most
   common reason" is outside the boss process, but I'd leave it to you.
 - As for the description of BIND10_LOST_SOCKET_CONSUMER, if the
   applications are generally expected to gently release the requested
   sockets before shutting down themselves, I'd clarify that
   "terminated" is something like unexpected crash or something.

 > >  - I don't know how exactly this method is used, but
 > >    get_socket()/sendall()/send_fd() could block, right?  [...]
 >
 > Yes, they could block. Actually, get_socket can't block, it's get_token
 which can, as it talks to the socket creator.
 >
 > However, I believe that:
 [...]
 > On the other hand, if we were to make sure they don't block, the
 handling around select would become significantly more complicated or we
 would need to introduce threads, with some kind of locking, etc.
 >
 > My assumptions may be wrong, but if they are right, then solving the
 problem would be waste of time. So I'd propose waiting with the solution
 until we find out it is a problem. Would it be OK?

 My experiences tell me that such optimistic assumptions on system
 reliability are often broken with a surprise in production systems of
 the real world.  But in this case I agree that the possible concerns
 do not outweigh the additional implementation complexity (in fact I
 didn't mean we should address this particular blocking issue within
 this ticket).  My suggestion is to leave some comments about
 considerations on blocking for now.

 > > - socket_consumer_dead()
 > >   - is it okay not to catch exceptions?
 >
 > Ups, you're right. It's maybe even worse than you thought, as the
 exception can happen in normal operation and is perfectly OK. Added.

 Okay, but is it okay to only catch ValueError?

 > >   - if the argument to drop_application() is the same one as the
 > >     'application' parameter of get_socket(), the assumptions seem to
 > >     be consistent: here it assumes to be an FD; in the latter it seems
 > >     to be undecided yet according to the docstring.
 >
 > Hmm, I fixed the docstring in the cache. Or did you mean some other? The
 cache itself doesn't care, so it was actually decided in this ticket.

 Oops I meant "inconsistent" here...anyway, the revised version looks
 okay on this point.

 > > '''bind10_test.py'''
 > >
 > > - Maybe we should rather import everything from bind10_src now?
 > > {{{#!python
 > > from bind10_src import *
 > > #from bind10_src import ProcessInfo, BoB, parse_args, dump_pid,
 unlink_pid_file, _BASETIME
 > > #import bind10_src
 > > }}}
 >
 > The lower import of bind10_src is there because I need to replace a call
 from library that one uses, this one:
 > {{{#!python
 > bind10_src.libutil_io_python.send_fd = self.__send_fd
 > }}}
 >
 > I'm not sure if it worked if it was imported by * (eg. if * imports the
 things the other module imports into itself), but even if it did, I thing
 it would be quite unreadable. This intention is more visible I believe.

 It just looked a bit awkward to me to have two imports of the same
 module, at least in that the policy of whether to omit the
 "bind10_src" prefix is inconsistent.  Not a big deal anyway, but may
 be adding comments like this?
 {{{#!python
 # In some cases we allow omitting "bind10_src" for brevity, in some
 # other cases we'll be more explicit to make it clear it belongs to
 # the bind10_src module.
 from bind10_src import ProcessInfo, BoB, parse_args, dump_pid,
 unlink_pid_file, _BASETIME
 import bind10_src
 }}}

 > > - "get_socket" test: I'd use an IPv6 address for this test (so we can
 > >   cover both the future and mostly-deprecated versions of IP).  We'll
 > >   need to replace _socket_cache.get_token to make it meaningful.
 >
 > Hmm, I'm not sure if I got the correct idea which test you mean. The
 stuff in this ticket only passes the address through (OK, it creates the
 IPAddr from the string), so it doesn't really matter which address it is.
 It is distinguished in the IPAddr class (which is tested) and deep down in
 the socket creator class that encodes it to the socket and sends it to the
 socket creator process (which is tested too). Did you mean this test, or
 another?

 I meant this one:
 {{{#!python
         bob._get_socket = get_socket
         args = {
             "port": 53,
             "address": "0.0.0.0",
             "protocol": "UDP",
             "share_mode": "ANY",
             "share_name": "app"
         }
         # Test it just returns whatever it got. The real function doesn't
         # work like this, but we don't want the command_handler to touch
 it
         # at all and this is the easiest way to check.
         self.assertEqual({'result': [0, args]},
                          bob.command_handler("get_socket", args))
 }}}

 And, recognizing you changed the IPv4 address used in
 TestCacheCommands to an IPv6 address, what I would have suggested is
 something like an attached diff.

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