BIND 10 #805: Client side code to request new sockets (C++)
BIND 10 Development
do-not-reply at isc.org
Wed Jan 4 15:03:04 UTC 2012
#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 |
-------------------------------------+-------------------------------------
Changes (by vorner):
* owner: vorner => jinmei
Comment:
Helo
Replying to [comment:14 jinmei]:
> 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.
That sounds reasonable.
I must admit I did this branch in a way to do minimal impact on the rest
of the code, which is not always for the best, and even more when I just
kept misusing stuff which was there already for slightly different
purposes. Even the old code had some of the problems you describe and I
agree it would need some bigger refactoring and redesign. Also, the
current approach doesn't use some features of the socket creator, like
being able to have multiple auth servers sharing a socket (which is not
possible for other reasons as well now).
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.
> - 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.
I find the way with member less intuitive/harder to imagine what is going
on, so I went for the way I used it. Also, the code would be longer to
write at the time when I needed the change. But provided C++ misses the
tools to do it cleanly (like mixins of D or roles from perl6) and you
already wrote it, we can as well use your version, which is nicer code-
wise.
> - 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.
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.
> - 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.
Yes, that one grew too much.
> - 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.
It could be avoided in that way, but the branch would get even bigger than
now. The way it is, the code needed for the test is quite short, but, as
you say, not really nice. I'd be for leaving this out until we decide how
the new interface should look like, it may turn out it wouldn't need such
measures for the tests anyway.
> - 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).
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?
> 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.
Yes, as I say above, I'd agree with this solution as well. I'd like to see
if it works at last to some extent.
> - setAddresses(): what's the magic string of "dummy_app"?
The share name for the socket creator. As the share mode is set to
DONT_SHARE, it doesn't matter, so I didn't like to introduce a way to
configure the current application name for now. That's why there's
"dummy_app" to fill something into the parameter, at last until we start
using the sharing somehow. I added a comment explaining it.
> - 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.
> 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.
It was, as the constructor only stored references to them and they were
not used before being initialized, but as you say, your way doesn't have
this problem anyway.
> - I suspect this TODO is now even more impossible.
> {{{#!c++
> // TODO Maybe some test to actually connect to them
> }}}
Well, we could fake the test socket requestor to give us a real socket and
then try it. And even if the TODO is hard, I believe it should still stay
there, because it is a valid test improvement.
> - 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);
> }}}
Hmm, right, it could be.
> - 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.
> }}}
I meant returns like returns the socket back to the socket creator/boss,
but you're right this could be confused with return as from function.
Releases is better.
> - 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.
Well, because I don't now how to handle the failure mode (I don't think
there's any other way than just kill the application), I don't check for
it (by the principle „don't check for errors you're unable to handle“).
But maybe having the case we don't know how to handle is a signal it needs
redesign as well.
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.
--
Ticket URL: <http://bind10.isc.org/ticket/805#comment:18>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list