BIND 10 #641: Memory leaks in NSAS

[BIND 10 Development]

#641: Memory leaks in NSAS
-------------------------------------+-------------------------------------
                 Reporter:  vorner   |                Owner:  stephen
                     Type:  defect   |               Status:  reviewing
                 Priority:  major    |            Milestone:  R-Team-
                Component:           |  Sprint-20110316
  Unclassified                       |           Resolution:
                 Keywords:           |            Sensitive:  0
Estimated Number of Hours:  0.0      |  Add Hours to Ticket:  0
                Billable?:  1        |          Total Hours:  0
                Internal?:  0        |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => stephen


Comment:

 Hello

 First, about your concern if NSAS having only raw pointer to the resolver.
 If we assume that the only client of NSAS is the resolver itself, then it
 should be safe ‒ the NSAS has some code running only when the resolver
 requests it (either by asking for IP address or by calling a callback). We
 must ensure that we don't delete the resolver while it has any methods
 running, even in multithreaded version of resolver, therefore it should be
 OK. And the assumption seems reasonable to me (who else should be using
 NSAS?).

 Anyway, in case we delete them both, the resolver should make sure all the
 pending callbacks are called, just like in the test resolver. But as this
 happens on shutdown only now, we probably can leave it out for now.

 Secondly, the code. There are two minor nits:
  - In nameserver_address_store.cc, the newZone function ‒ there's a
 comment everything is passed as a pointer, so the compiler has it easier
 to prove it doesn't need to call the copy constructors of the shared
 pointers because of their side effects (which I believe it can't, in case
 of multithreaded application, because the ref count is different). If
 there was only a pointer (a primitive type), it knew there are no
 sideeffecs, allowing it to completely eliminate the boost::bind call to
 create a functor, inlining the whole thing. There's no longer need to have
 double pointer to the resolver once the there's no shared pointer.
  - The exceptions of random number generator ‒ I believe they should have
 a common ancestor, but something less general, than just Exception, so
 someone might just call all the „bad input of random number generator“ at
 once, without listing all of the cases, but without catching all
 exceptions. Maybe that common ancestor should also be subclass of
 InvalidParameter or BadValue.

 Is it OK to have no changelog entry? Maybe yes, because the resolver
 didn't really exist in the previous releases in usable state, so a memory
 leak in the tests wasn't a bug worth noting.

 With regards

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