BIND 10 #1747: refactor auth::Query so it's reusable

[BIND 10 Development]

#1747: refactor auth::Query so it's reusable
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  high   |  Sprint-20120320
                  Component:         |            Resolution:
  b10-auth                           |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  4
Feature Depending on Ticket:  auth   |           Total Hours:  0
  performance                        |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:5 jelte]:

 First, regarding this point:

 > Regarding these vectors; process() is not exception-safe (just like it
 was not and is not safe because it modifies response); they can be
 modified. However, these vectors are reset both in createResponse and
 before process() starts to do any work, which i think for now is good
 enough.

 In my definition of exception safety it's safe in that it doesn't
 cause such disruption as memory leak on exception; but it doesn't
 provide the strong exception guarantee (either complete success or
 fall back to the initial state on exception).  I interpret you meant
 this, and that's true.  And, in fact, it's probably almost impossible
 to provide the strong guarantee for the response message (unless we
 prepare a temporary copy beforehand, which would be too costly), so I
 think it's acceptable.

 Regarding the internal buffers, I think it's relatively easy to
 guarantee that: We can use a small helper "scope" object

 {{{#!c++
 class QueryCleaner {
 public:
     QueryCleaner(auth::Query& query) : query_(query) {}
     ~QueryCleaner() { query_.reset(); } // we need to make reset public
 private:
     auth::Query& query_;
 };
 }}}

 And we encapsulate the query object before calling process(), and let
 `QueryCleaner` clean it up either on success or exception:

 {{{#!c++
         if (memory_client_ && memory_client_class_ ==
 question->getClass()) {
             const RRType& qtype = question->getType();
             const Name& qname = question->getName();
             QueryCleaner cleaner(query_);
             query_.process(*memory_client_, qname, qtype, message,
 dnssec_ok);
         } else {
 }}}

 But realistically it's quite unlikely we see exception from process()
 with the in-memory data source, so if you want to defer it to a
 separate task, it's okay for me.  At least as long as the vectors are
 cleared at the beginning of process() there shouldn't be anything
 wrong even if an exception happens.

 Comments on the implementation follow:

 '''misc'''

 - I made a few minor editorial/style fixes to the branch.
 - do we need a changelog?  maybe not because it's mostly an internal
   refactoring, but I'm asking anyway since it still changes the
   interface given in a public header file.

 '''higher level points'''

 - Query now seems to be copyable.  Is that okay?  There's probably no
   particular reason it cannot be so, but if we don't have a reason we
   want to make a copy of it, it's probably safer to make it
   noncopyable (with noting these) to avoid making an unexpected copy.
 - Why is Query::qname_  (and the qname parameter of process()) a real
   object?  This will cause a relatively expensive copy for every call
   to process().  It's also now mutable (while it still doesn't seem to
   have to be so), which makes the code more fragile (we could
   accidentally override it in the middle of process()).  If there's no
   particular reason, I'd suggest changing it to 'const Name*'
   process() and initialize() could take 'const Name&' and set qname_
   to its address.
 - Same for qtype_ and dnssec_.  Also, especially for qname_ and
   qtype_, I'd be nervous about their initial values.  While I
   understand if we really need to keep a real object there's not many
   choices and sometimes we end up using arbitrary initial value.  But
   such ad hoc values often lead to bugs - I basically want to keep
   things "undefined" when they don't have real meaning.  Maybe we can
   consider using boost::any (not sure about its performance
   impact)...continue to the next point:
 - Considering these, I wonder whether we need to keep the query
   parameters within the Query class.  From a quick look they are only
   for the convenience of the support private methods - we could also
   pass these parameters to these methods instead of storing them in
   Query and have the private methods refer to them (right)?  If
   passing around many parameters causes readability issue, we can
   think about introducing a helper structure, say QueryParam, which is
   essentially a read only tuple of the query parameters.  The Query
   class could either hold it as a pointer (and initialize reset it to
   NULL except while doing process()) or pass that object to private
   methods (then there'll be only one additional parameter).  The
   QueryParam can be constructed for every query, so if we need to make
   a copy of Name/RRType we don't have to use placeholder initial
   objects for them.

 - I guess we might want to trim excess vector capacity in case it
   happen to hold unusually large number of items:
 {{{#!c++
 const size_t MAX_RESERVE_RRSETS = 64; // note: arbitrary choice
 if (answers_.size() > MAX_RESERVE_RRSETS) {
     vector<ConstRRsetPtr> new_answers;
     new_answers.reserve(MAX_RESERVE_RRSETS);
     answers_.swap(new_answers);
 }
 }}}

 '''query.cc'''

 - We can remove most of the boost::const_pointer_cast (in fact we can
   consolidate them within `RRsetInserter`).  (Of course, the need for
   it itself is bad even if it's used "once" and we should eventually
   solve this, but that's another issue).  I believe this fix is
   trivial so I made it to the branch directly.

 - maybe a matter of taste, but I'd let processDSAtChild() call
   createResponse() internally and keep the caller doing nothing.
   Especially for the case of DELEGATION, since this switch is pretty
   big (which I think would also require refactoring, but that's
   another issue), it makes me nervous to see the break because it's
   not so obvious from the code around the "DELEGATION" case what
   happens after 'break'.  Note also that with the current convention
   we'd also have update the documentation in the header:
 {{{#!c++
     /// It returns true if this server has authority of the child;
 otherwise
     /// it returns false.  In the former case, the caller is expected to
     /// terminate the query processing, because it should have been
 completed
     /// within this method.
 }}}
   because the "In the former case,..." is not very accurate any more.

 - This createResponse() doesn't seem to be necessary:
 {{{#!c++
         response_->setHeaderFlag(Message::HEADERFLAG_AA, false);
         response_->setRcode(Rcode::REFUSED());
         createResponse();
         return;
 }}}

 - If we copy objects in initialize(), we could just pass references to
   process() and initialize(), then we can save a couple of copies
   (although the compiler may optimize these one-shot copies).  But see
   also the design level discussions above.

 '''query.h'''

 - I'd say "message" instead of "packet" (my usual suggestion in such a
   context:-)
 {{{#!c++
     /// This will take each RRset collected in answers_, authorities_, and
     /// additionals_, and add them to their corresponding sections in
     /// the response packet.
 }}}

 - Maybe "Default constructor" is better than "Empty constructor"?
 {{{#!c++
     /// Empty Constructor.
     ///
     /// Query parameters will be set by the call to process()
     ///
     /// This constructor never throws an exception.
     ///
 }}}
   Also, technically, it could throw in constructing qname_.

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