BIND 10 #408: General control logic of NSAS

[BIND 10 Development]

#408: General control logic of NSAS
---------------------------+------------------------------------------------
      Reporter:  vorner    |        Owner:  vorner   
          Type:  task      |       Status:  reviewing
      Priority:  major     |    Milestone:           
     Component:  recurser  |   Resolution:           
      Keywords:            |    Sensitive:  0        
Estimatedhours:  0.0       |        Hours:  0        
      Billable:  1         |   Totalhours:  0        
      Internal:  0         |  
---------------------------+------------------------------------------------

Comment(by vorner):

 Replying to [comment:6 stephen]:
 > ''ZoneEntry::process()''[[BR]]
 > The header says that this function "processes the nameservers".  What
 processing is done?

 I hope this explanation is better.

 > Could we have a bit more documentation on the locks.  Under what
 circumstances should the caller pass a lock and under what circumstances
 should the zone entry's own lock be used?  Also, the header says that if
 an exception is generated the lock is not released; what if the lock is
 the internal one?

 The working of locks was probably too complicated. I found out that the
 problem/complications may be solved by using a recursive lock, so I
 changed that one and removed the parameter. So we will have less
 documentation on the locks O:-).

 > Is there really a need for the process guard?  If there are multiple
 calls on the stack, as each trigger a callback only those callbacks queued
 will be executed.  Is this a bad thing?  (If this is being called in
 response to the completion of some external fetch operation - the
 documentation is not clear here - do we want to wait until all outstanding
 fetches have completed?  Some may take a long time and some may not result
 in an answer being returned.  In this case, it is better to call all
 callbacks with the information that arrives first.)

 There is. It waits only for the nameservers already in cache, not for the
 ones from external fetches. I changed the comment explaining why it is
 done.

 > '''src/lib/nsas/fetchable.h'''[[BR]]
 > Setting a default value of NOT_ASKED for the constructor argument would
 have saved the need to define a default constructor.

 Thanks, good catch.

 > '''src/lib/nsas/TODO'''[[BR]]
 > Comment on cache notifying NSAS of changes: the only changes the NSAS
 should know about is (a) when a set of nameservers changes (this is most
 likely if the delegation RRset in the parent is different from that in the
 zone itself) and (b) if zone information is explicitly deleted from the
 cache (in which case the zone entry should be deleted).

 OK

 > Comment on removal of LRU from nameserver entries: using a weak pointer
 in the LRU list is a good idea.  The nameserver entry already contains a
 pointer into the LRU list; so a simple solution is to add something to the
 destructor that removes the entry from the list.  However, it's not clear
 how to prevent a concurrent accessor looking up that nameserver entry in
 the hash table while the destruction is in progress.  The LRU list
 approach works; setting the size to about 3-4x that of the zone entry LRU
 list should avoid nameservers being deleted while their zones are active;
 few zones have more than four nameservers and many have less.

 Actually, I do not want a LRU list with nameservers at all. I want only
 the hash table with weak pointers. The thing keeping them alive would be
 the ZoneEntry holding the nameserver.

 The with weak pointers there would delete them safely (shared_ptr
 promisses that). The gotcha there is the weak pointer is left inside the
 hash table and throws an exception upon access. So we would need to catch
 that exception when we access that hash slot and delete the dead weak
 pointer (which points nowhere at that time).

 And the LRU list approach probably does not work, because an active zone
 entry never touches the hash table again if the nameserver list stays the
 same. It keeps the original pointers, avoiding lookups. Therefore we will
 drop a nameserver of the oldest zone entry, but that one might still be
 active.

 > Comment on dispatching callbacks: either remove and dispatch one at a
 time, or catch all exceptions and not if any were raised; after all
 callbacks have been dispatched if an exception was caught, raise a new
 one.  But as you say, what happens if an exception is raised in a callback
 - will the program continue?

 Catching all exceptions does not work in C++. catch (exception &) catches
 only the ones inheriting from exception, while anything in C++ can be
 thrown. And catch (...) does not give the thing we caught and so we have
 nothing to throw again. And there's still the problem of two concurrent
 exceptions. Probably taking out one at a time with the recursive locks
 will work better.

 > '''src/lib/nsas/nameserver_entry.h'''[[BR]]
 > '''src/lib/nsas/nameserver_entry.cc'''[[BR]]
 > Is there any reason why the header commentsfor askIP() are delimited by
 /*...*/?  All other comments in the .h file use the Doxygen "///" form.

 Well, yes, there is.

 For one, it is /** ... */, which is a Doxygen comment as well as ///. It
 is just multiline one. I tend to use multiline comment form with multiline
 comments, for one, it is clear that it is a single comment and not
 multiple short comments one after another. Second, vim does not seem to
 support /// comments very well. The block alignment (gq) puts /// in the
 middle and it puts only two slashes after pressing enter. I know this
 looks little bit inconsistent with the other comments, but using a single-
 line comments for one multiline seems strange and modifying all the old,
 already written ones, seems wrong too.

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