BIND 10 #513: b10-auth hangs in submitting statistics

[BIND 10 Development]

#513: b10-auth hangs in submitting statistics
-------------------------------------+-------------------------------------
                 Reporter:  jinmei   |                Owner:  jinmei
                     Type:  defect   |               Status:  reviewing
                 Priority:           |            Milestone:  A-Team-
  critical                           |  Sprint-20110126
                Component:           |           Resolution:
  b10-auth                           |            Sensitive:  0
                 Keywords:           |  Add Hours to Ticket:  0
Estimated Number of Hours:  0.0      |          Total Hours:  0
                Billable?:  1        |
                Internal?:  0        |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:8 y-aharen]:
 > I've completed my review.

 Thanks!

 > - src/bin/auth/auth_srv.cc:
 >
 > {{{
 > 109     /// Interval timer for periodic submission of statistics
 counters.
 > 110     /// When NULL, the submission is disabled.
 > 111     IntervalTimer statistics_timer_;
 > }}}
 > !IntervalTimer can't be NULL, it's not a pointer. I think the comment on
 line 110 can be deleted because the way to disable timer is described in
 the other place.

 Ah, good catch.  It was originally a pointer, but I modified it to a real
 object in a later phase of development, and forgot to update the comment.
 Fixed in the branch.

 > - I think the interval value passed through cmdctl should be checked
 that is not negative (i.e. not < 0). In the current implementation, we can
 set Auth/statistics-interval to -1. It causes infinite invoking of call
 back function. The parameter check can be done in
 StatisticsIntervalConfig::build(). If the value is negative, it should
 throw !AuthConfigError or treat as 0 to disable timer. The choice of
 solution is up to you.

 Another nice catch.  Fixed.

 Please let me know if there are other issues or the branch is ready to
 merge.

 (The following are notes that are not directly related to this ticket)

 Although I agree it should be rejected at the frontend of configuration
 parser (so I fixed it), I also believe this is primarily a problem of
 the interface of IntervalTimer::setupTimer().  It takes an unsigned
 integer and naively passes to boost::posix_time::seconds(), which assumes
 a signed long value.  We should at least use a fully compatible type
 (simple 'long' would be best because we cannot even assume it's 32 bit),
 and, if we want to reject negative timer intervals as part of the
 IntervalTimer interface, we should explicitly check and reject them
 in its setupTimer().  But I wouldn't go into that level within this ticket
 anyway.  It would have to go to a separate ticket/task.

 For that matter, while I extended the IntervalTimer class, I noticed
 the name of "setupTimer()" was redundant because the class name already
 clarifies that it's somehow related to the timer.  As a hindsight it
 should have been named "setup", and so I chose "cancel()" rather than
 "cancelTimer()", which might be better in terms of consistency.
 So, for consistency and conciseness, I'd suggest renaming "setupTimer()"
 to "setup()" (or at least something not redundant).  It should be
 a topic of separate ticket, though.

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