BIND 10 #446: configuration knob for in memory data source

[BIND 10 Development]

#446: configuration knob for in memory data source
---------------------------+------------------------------------------------
      Reporter:  jinmei    |        Owner:  vorner               
          Type:  task      |       Status:  reviewing            
      Priority:  major     |    Milestone:  y2 12 month milestone
     Component:  b10-auth  |   Resolution:                       
      Keywords:            |    Sensitive:  0                    
Estimatedhours:  0.0       |        Hours:  0                    
      Billable:  1         |   Totalhours:  0                    
      Internal:  0         |  
---------------------------+------------------------------------------------

Comment(by jinmei):

 Replying to [comment:3 vorner]:
 > Few comments:
 >
 Thanks, these are good ones:-)

 >  - Why does destroyAuthConfigParser exist? When I saw it, I asked „Huh,
 does destroying a config parser need special attention? Why shouldn't I
 just call delete? What would go wrong?“. As I looked inside it, it does
 just that, so I think this one is only confusing.
 >
 I knew this would look oakward, but technically we cannot simply let a
 module delete an object that were newed in a different module, because
 these two may use incompatible implementations of new/delete.  That's the
 reason why I introduced the dedicated "destroy" function.

 In practice, however, I admit this may be overkilling because normal
 applications are not supposed to create AuthConfigParser directly as
 documented (createAuthConfigParser() is provided as a public function
 mainly for testing purposes).

 Another "tecnically correct" solution is to use a smart pointer and have
 createAuthConfigParser() return a smart pointer version of the new'ed
 object; the caller can simply forget the deallocation.  I didn't choose
 this path to reduce the dependency on boost (the only realistic choice of
 smart pointer in this context would be boost::shared_ptr), but, on
 rethinking it, it may be okay as this interface is supposed to be used
 only within our own implementations (and the caller would be tests).

 I can think of several possible ways to address your concern:

  1. no code change, but add document about the reason for having a
 separate function
  2. have the caller delete the returned pointer and document it with the
 pitfall of new/delete mismatch; remove destroyAuthConfigParser.
  3. use a shared pointer and remove destroyAuthConfigParser.

 Does any of the options address the concern?  If so, what do you think is
 the best?  If not, do you have a suggestion?

 BTW:

 > Anyway, this function not only should never throw, it just can't, C++
 specifies that if anyone tries to throw out of a destructor, the
 application aborts.
 >
 I guess you are talking about this part of doc:
 {{{
 /// This function is not expected to throw an exception unless the
 underlying
 /// implementation of \c parser (an object of a specific derived class of
 /// \c AuthConfigParser) throws.
 }}}

 and you mean that, since the only thing destroyAuthConfigParser() does is
 to do delete (which effectively means calling a destructor in this
 context), if \c parser throws it should be from a destructor.  If so,
 that's correct.  But I'm not sure what you mean by the "C++ specifies ..."
 part.  As far as I know, it's not prohibited by the language spec,
 although it's widely considered a very bad practice.  For example, if you
 run this code:

 {{{
 struct BadThrower {
     ~BadThrower() { throw 0; }
 };

 int main() {
     try {
         BadThrower bt;
     } catch (const int& ex) {
         std::cout << "bad practice, but possible" << std::endl;
     }
     return (0);
 }
 }}}

 We'll see this: "bad practice, but possible".

 >  - Isn't it a lot to ask from commit not to throw any exception? Is it
 realistic to say that none of them will ever allocate memory, for example?
 >
 Yeah, I know it can be a lot.  But if we want to implement some way of
 "safe configuration update", we need some strong requirements like this.
 By "safe" I mean if an attempt of configuration update fails the old
 configuration remains valid intact.  In other words the commit() variants
 are a kind of no-throw swap(), which can sometimes be difficult to
 implement but is crucial in some scenarios.

 And, since commit() assumes a strong requirement, I separated it from
 build(), which can throw and would often need to allocate memory.  The
 idea is to have build() do any work that can fail without touching the
 server object as much as possible, and then commit() completes the task
 with much restricted, but safer primitives.

 If we think the requirement is too restrictive and unrealistic, possible
 alternatives would be:
  1. introduce a "cancel()" method (or have the destructor do it), which is
 responsible for rolling back incomplete configuration committed in the
 server.
  2. just let commit() throw when it can't avoid that, with a note that the
 system may become inconsistent state in that case

 IMO option 1 would actually be as strong as requireing commit() not throw,
 so I suspect it's not a realistic option.  Option 2 sound irresponsible to
 me, and I don't like it, but depending on how we handle the configuration
 stuff in practice, we may be able to live with it as a compromise.

 What's your opinion?

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