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):

 Replying to [comment:18 vorner]:

 In short, I'd give this branch a go.

 I made a few more changes while checking the updates.  Most should be
 trivial, but please note 3fcadc1a85ca56620c418e8b18cf43e43afbe98d,
 which fixed a real regression.  Ideally this should have been caught
 by a test.

 > I don't know if immediately after the release is the best time, though.
 In case we would go for some more drastic receptionist approach, it could
 render the portconfig module deprecated, so it would make more sense to
 just keep it in the proof-of-concept state for a month or two before the
 receptionist would be implemented.

 My concern with this approach is that merging something to master will
 create inertia against further cleanups.  If we allow an option of
 (possibly) cleaning them up "some time later", that some time will
 never come and we'll suffer from dirty state of the design and code.
 IMO, even if it may end up being thrown away, making all efforts
 waste, we shouldn't allow keeping the master code too dirty for an
 undecided period.  So I'd insist either: merge this with a promise of
 cleaning up very soon, understanding it can delay development of other
 stuff such as DDNS or NSEC3, or don't merge.  I believe we need
 discipline before merge to avoid the bad inertia.

 Which level of cleanup is necessary can be discussed, though.

 As for the receptionist approach, just like the case with msgq
 replacement I don't like to use it as an excuse to skip the cleanup;
 we don't even know whether we adopt it, much less whether it really
 solves the issues we are thinking about now.

 > > - 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.  [...]
 >
 > It is singleton now for few reasons:
 >  * When I started implementing it, I started with a namespace with
 functions, like in the case of portconfig. I thought of it like a
 portconfig extension. But it turned out it would be hard to test without
 the ability to subclass it and that there could be some variables.
 >  * I don't see a reason why a real application would need more than one
 of it.
 >  * I didn't want to pass the object around, at last not at the time of
 writing it, because it would need to modify too many files, while using it
 as a global variable allowed me not to change the interface of portconfig.

 To me these cannot be a justifiable reason for relying on a global
 instance (which is what singleton is effectively).  I don't request it
 be changed for now, but if these are the reason I'd more strongly
 suggest revisiting it as part of the cleanup.

 > > - personally I'd hide gtest many of the function definitions from the
 > >   header file.  [...]
 >
 > But then, I'd need a .cc file to place the methods with the EXPECT_*
 macros into. And it would introduce a problem with ordering of library
 compilation, as the testutils library would need to use the server_common
 library and the server_common tests would need to use the testutils
 library. Therefore we couldn't have the tests as a subdirectory of
 server_common, but the lib/Makefile.am would need to include something
 like:
 > {{{
 > SUBDIRS = … server_common testutils server_common/tests
 > }}}
 >
 > which looks ugly.
 >
 > Or is there a solution I don't see?

 Ah, okay.  So the mutual dependency is the more fundamental problem.
 Basically, when I first created this helper library (I think it was
 me) I didn't intend to introduce a reverse dependency from this
 library to higher level modules such as auth/resolver servers.
 server_common is essentially part of these applications, so something
 that depends on server_common in the testutil lib breaks the
 assumption in the first place.  IMO this should go (at least
 conceptually) to somewhere around server_common/tests and
 auth/resolver tests refer to it, independently from whether or not to
 move the definition to .cc.

 I suggest this should be cleaned up in a later ticket, too.

 > > - 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.
 >
 > Well, failure to release them would mean either of:
 >  * serious bug in the communication, which should be fixed soon
 >  * boss disappeared
 >  * msgq disappeared
 >
 > These are extremes under which we would probably exit anyway, and I
 didn't see an easy way to do it in a more reliable way. Requesting (and
 returning) sockets from/to different application seems cumbersome as of
 itself.

 That's probably true that they are extremes, but the common practice
 is that releasing resource (close(), free(), delete, etc) should
 generally be 100% error free even considering such extremes.  So IMO
 we should basically try to meet the common expectation.

 I think that in the possible redesign of port config, we should
 separate the error free part and other related operations that can
 fail.  The former is local only operations such as closing the socket
 or releasing the corresponding server object.  Once making that sure,
 we can perform the other part, taking into account failure cases and
 whether or how to recover from it.

 But as you said, it's also true that the idea of the socket creator
 approach itself is cumbersome.  We are now seeing specific cases
 around it, and the end result may be that it's not a good idea after
 all for production systems.  It will be a pity if that's a case, but I
 think it's okay as long as we are allowed to use time for such
 try-and-errors - there are something we cannot know until we actually
 play with them.

 > I don't think we can come up with anything significantly better than
 this, if we are about to make it in time to release. So the question is,
 should we include this? I guess yes, because even a possibly suboptimal
 user-visible feature is still user-visible and gives an impression we do
 something. So, do you see any other problems that are not hard-to-solve
 design issues? We might maybe find a short time to discuss what we want
 from the configuration subsystem during the F2F, at last in 2 or 3 people.

 As stated above, I'm okay with including it if we make a promise of
 making the follow up cleanups very soon at a higher priority.

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