BIND 10 #805: Client side code to request new sockets (C++)

[BIND 10 Development]

#805: Client side code to request new sockets (C++)
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20120110
                   Priority:         |            Resolution:
  blocker                            |             Sensitive:  0
                  Component:         |           Sub-Project:  DNS
  Unclassified                       |  Estimated Difficulty:  5
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
  Socket creator                     |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 I've completed my review.

 First, I made a few minor changes directly to the branch which I
 believe are correct and/or non controversial.

 Second, while looking at details, a lot of design level
 questions/suggestions/concerns arose.  I think these are substantial
 (at least to me) to some extent, but I'm afraid we don't have time to
 resolve all of them before the release (even if the "resolution"
 results in rejecting the issue after discussions).  And, in some sense
 many of these points are minor in that it would work somehow as an
 initial proof of concept.  So, maybe the possible way forward is to
 only address easy points and merge the result, then work on more
 fundamental points immediately after the release.

 '''higher level points'''

 The following points are some of the design level points.  I'd like to
 resolve the first one before the release (I have a concrete proposal
 so if we agree with it that part is easy), but the other two can be
 deferred.

 - I have to say I don't think it a good idea to use multiple
   inheritance for TestSocketRequestor.  While I'm generally more
   convinced that multiple inheritance does more harm than good in many
   cases, I'm also willing to be flexible and can live with it if it
   really makes the design elegant which couldn't be realized in other
   ways.  Still, in this specific case, it seems to me an unnecessary
   abuse of it.  I have a counter proposal of doing the same thing
   using more straightforward composition while avoiding (diamond
   style) multiple inheritance.  Please see the attached diff.

 - although this is not in the direct scope of this branch: while
   thinking about various issues I encountered in the branch (detailed
   below), I now wonder why we need to make SocketRequestor a singleton
   by design.  Singleton is a very strong idiom and inherits many bad
   properties of global variables (the fact that we need to use
   something like "initTest" for tests is one of them), so we'd rather
   avoid relying on it unless we have a strong need for it.  In our use
   case, the application would still need to explicitly create a
   specific instance either by SocketRequestor::init() or instantiating
   TestSocketRequestor, we could make the init function a factory
   (returning an instance) and use a pointer or reference to it
   wherever we need to get access to it.  Then the most part of the
   application code won't have to be aware that SocketRequestor is a
   singleton, and, in particular, we should be able to simplify tests
   (we'd be able to just create and discard a test requestor for each
   test case).  If we still want to make sure the requestor is never
   instantiated twice in normal applications by interface, we could
   implement that logic in the factory function.

 - more or less related to the above two points, it seems to me the
   portconfig logic is getting more complicated with various states (at
   least the requestor and "current_sockets" were newly introduced
   states that are cooperating in the same context), so I wonder it
   makes more sense to introduce a class to enclose the configuration
   logic.  Then we'd construct a "configurator" object with a pointer
   or reference to the socket requestor (thereby eliminating the need
   for getting access to the requestor via the singleton interface) and
   have the object maintain "current sockets" as a class attribute
   (thereby eliminating the use of semi-global namespace scope static
   variable).  But, aside from whether this idea is a good one, I also
   think we need to revisit how to configure listen ports in general
   (detailed below).  So this point should be considered as part of
   revisiting the entire scheme.

 '''(test) socket_request.h'''

 - can't we avoid using the global portconfig::test_mode variable?  I
   understand we sometimes have to sacrifice the cleanness for
   testability as a matter of practice, but this variable is accessed
   far from the original module (i.e. server_common) and it's difficult
   to track the dependency.  We could, for example, introduce an
   abstraction of DNSService and use a special derived class of it for
   tests, where addServerXXXFromFD does nothing.
 - personally I'd hide gtest many of the function definitions from the
   header file.  in particular, I'd avoid directly relying on gtest
   specific definitions in a header file (and including gtest.h, which
   subsequently imports many other definitions including macros and
   will make this header file more susceptible to ordering issues).

 '''portconfig.cc'''
 - use of namespace scope static variable doesn't seem to be good:
 {{{#!c++
 vector<string> current_sockets;
 }}}
   both in terms of the risk of having initialization fiasco and of
   introducing a semi-global mutable variable, which is difficult to be
   tracked (causing bugs, making debug more difficult).

 - `setAddresses()` doesn't provide the strong exception guarantee (for
   current_sockets), and if something goes wrong in the middle of this
   function current_sockets and the boss will (possibly) have some
   inconsistent states.  In this context note also that even
   releaseSocket() could fail and throw; it's not a usual resource
   release function (which would generally be considered exception
   free), but involves many complicated things that can fail, including
   message creation and network communication.  Further, the entire
   logic of the port configuration doesn't seem to be a good one to me
   in the first place.  It first clears all server objects (so the
   corresponding sockets will be closed) even if we are reusing the
   some of the address/port pairs.  This could lead to dropping queries
   even if the server could otherwise handle.  We should at least avoid
   the unnecessary close-rebind process.  Also, as commented in
   installListenAddresses(), it doesn't seem to be the best behavior to
   try to use the all or nothing policy here.  We can discuss this, but
   my expected behavior would be to handle each address-port config
   separately and if some of them fails we basically keep going with
   others (while leaving error logs for the failure).  For that matter,
   that's how BIND 9 works, too.

   Frankly, I don't know what we should do with thin in the mean time.
   All these weird things seem to suggest that it's time to revisit the
   whole design of port configuration, especially with taking into
   account the introduction of the socket creator.  On the other hand,
   I don't think we can complete that task before the coming release.
   One possible compromise might be to introduce this version for the
   release, ignoring all these issues, and immediately revisit the
   design after the release at the highest priority.

 - setAddresses(): what's the magic string of "dummy_app"?

 - While this is minor compared to the bigger issue explained in the
   bigger comment above, the recovery process in
   installListenAddresses() is now even more unreliable:
 {{{#!c++
         try {
             setAddresses(service, addressStore);
         }
 }}}
   because even releasing the address in setAddresses() may now fail.

 - I don't understand this comment:
 {{{#!c++
             // Releasing them should really work
 }}}
   (What does "should really work" mean?)

 '''auth_srv_unittest.cc'''

 - In the following way, dnss_ and address_store_ are passed to
   TestSocketRequestor before being initialized.
 {{{#!c++
     AuthSrvTest() :
         TestSocketRequestor(dnss_, address_store_, 53210),
 }}}
   It may or may not be (happen to be) safe depending on the details of
   TestSocketRequestor, but not a good practice anyway.  But more
   fundamentally I'd rather avoid using inheritance here (see the
   higher level issues above).  Note: this initialization issue exists
   in other cases where TestSocketRequestor() is initialized.  Note
   also that this problem doesn't exist in the proposed diff.

 '''portconfig_unittest.cc'''

 - I suspect this TODO is now even more impossible.
 {{{#!c++
     // TODO Maybe some test to actually connect to them
 }}}
 - Shouldn't the exception class be more specific?
 {{{#!c++
     // This should not bind them, but should leave the original addresses
     EXPECT_THROW(installListenAddresses(invalid_, store_, dnss_),
 exception);
 }}}
   Same for the brokenRollback test.
 - I don't understand this comment.  What's 'everything'?  Perhaps you
   mean 'releases' instead of 'returns'?
 {{{#!c++
 // Try it at least returns everything when even the rollback fails.
 }}}
 - in the following comment, "These" should be "the first pair"?
 {{{#!c++
     // These should be requested in the first part of the failure to bind
     // and the second pair in the first part of rollback
 }}}
 - I'd say there will be more error cases to test.  For example,
   there's a case when release fails.  But I'm not sure if we bother to
   be complete here, if our next action is to revisit the design
   fundamentally.

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