BIND 10 #401: Timouts in the recursor

[BIND 10 Development]

#401: Timouts in the recursor
------------------------------+---------------------------------------------
      Reporter:  vorner       |        Owner:  stephen  
          Type:  enhancement  |       Status:  reviewing
      Priority:  major        |    Milestone:           
     Component:  recurser     |   Resolution:           
      Keywords:               |    Sensitive:  0        
Estimatedhours:  0.0          |        Hours:  0        
      Billable:  1            |   Totalhours:  0        
      Internal:  0            |  
------------------------------+---------------------------------------------
Changes (by vorner):

  * owner:  vorner => stephen


Comment:

 Replying to [comment:7 stephen]:
 > '''recursor.h'''
 >  * Methods are documented, but not the class itself.  What is its
 purpose?

 Fixed

 > '''recursor.cc'''
 >  * RecursorImpl::!RecursorImpl should initialize rec_query_ to NULL
 (instead of "()".

 Fixed

 >  * Is there a potential failure mode here should querySetup() be called
 when a query is in progress?  Suggest that if querySetup() is called with
 a non-NULL rec_query_ an exception is thrown.

 Well, it is an error to call querySetup() when queryShutdown() wasn't
 called before, because it would leak the memory of previous RecursiveQuery
 object. I added an assert there (this is not something anyone should ever
 catch).

 Current code expects there are no threads here, because queryShutdown
 could delete the RecursiveQuery object while a call to its method would be
 running. That is problem of more of the code there, that it is not thread
 safe at all and isn't meant to.

 But if used from the same thread, calling delete on it is safe at any
 time. The sendQuery method just creates an autonomous object waiting for
 the answer, it is independent of the originating RecursiveQuery.

 >  * The classes !QuestionInserter and !SectionInserter seem to related
 more to code associated with general DNS message manipulation than with
 the recursor.  Should they be in recursor.cc or in the main DNS library?

 They do not seem as some utility commonly used. They just hack around C++
 inability to have lambda functions and for_each strange interface. If I
 wrote the code, I'd use a BOOST_FOREACH or put the classes directly inside
 the function that uses them. If I saw them in the public interface of the
 library, I'd be asking why these classes are there, when I can just insert
 into the data directly.

 >  * Similarly, !MessageAnswer does a general transformation of a message.
 Although it takes a Recursor* argument to its constructor, it seems that
 this is never used.  I would suggest that the functionality of
 !MessageAnswer is placed in the main DNS library and that the
 !MessageAnswer code just calls it.

 Yes, I removed the argument, it was a leftover from some older code. Maybe
 gcc should warn about private unused member variable as well.

 This one inherits from DNSAnswer, which is from asiolink. I guess the dns
 library should be able to work without asiolink, therefore it shouldn't
 contain such class.

 I'm not definite on these two, but having them locally unless someone else
 has need for them elsewhere seems to make more sense.

 > On a more general point, documentation: as the resolver progresses, it
 is getting more difficult to relate the bits to one another.  I think we
 need to take some time to document what we have so far.  At minimum a
 class diagram and an outline of the general logic is required.  (In
 fairness, I think that also needs to include the asiolink classes; there
 is some documentation in the "doc" directory but it does need to be
 extended.  Most of the time in this review was flitting back and forth
 between asiolink and recurse trying to work out what method was being
 called and why.)

 Should this be part of this ticket?

 Not that I wouldn't agree that there's need of documentation (it took me a
 while to read and remember how the code works and I'll forget it soon
 again), but I think it should go either into #327 or into a separate task.
 Is it OK if I just create a ticket or put this into the rteam backlog?

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