BIND 10 #957: IntervalTimerTest.destructIntervalTimer segfaults

[BIND 10 Development]

#957: IntervalTimerTest.destructIntervalTimer segfaults
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  y-aharen                           |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20110614
                   Priority:  major  |            Resolution:
                  Component:         |             Sensitive:  0
  Unclassified                       |           Sub-Project:  Core
                   Keywords:         |  Estimated Difficulty:  3.0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by y-aharen):

 * owner:  y-aharen => jinmei


Comment:

 Replying to [comment:10 jinmei]:
 > Replying to [comment:9 y-aharen]:
 > > Replying to [comment:7 jinmei]:
 > > > Normally we should first add a test case that could trigger the bug
 > > > without the proposed fix, fix the problem in the main code, and then
 > > > confirm the fix actually solves the problem with the previously
 added
 > > > test.
 > > >
 > > > Isn't it possible in this case?
 > > The segfaults will occur when !IntervalTimer is destructed while the
 timer has been expired and the invocation of the callback function is
 pending. I couldn't make such circumstances continually in unittest.
 >
 > I'd suggest adding some assertion check like the attached one.  This
 > would make the bug more likely to happen without this fix.
 Thank you for your helpful suggestion. I finally got what was happening in
 the destruction of an instance of the class. I modified the main code. In
 commmit 3e1fae0b23d4df32f239e5aa1350e40557d8ada4 fails on
 !IntervalTimerTest.destructIntervalTimer. In commit
 49ae23f163b8e4ad45f7146f95235253691a0f4b the test will success.

 > > I'd like to fix this to correctly manage the lifetime of an instance
 of !IntervalTimer by using boost::shared_ptr and shared_from_this(), which
 seems to be best practice.
 >
 > The code basically looks okay.  I have some comments and suggestions
 > however:
 >
 > - I'd like to see more detailed comment about why we need
 >   shared_from_this, and, if that's a common practice, with a reference
 >   to this technique along with ASIO.  I myself didn't even know the
 >   existence of shared_from_this much less the use of it with ASIO.  If
 >   it's not only me, we need deeper explanation to help others
 >   understand the code.
 I explained the detail with a link to asio documentation.

 > - I suspect this should never happen because the access to
 >   !IntervalTimerImpl is limited in interval_timer.cc.  If this ever
 >   happens, it means a severe internal bug.  So, I'd rather use
 >   assert() here.
 > {{{
 > +    } catch (const boost::bad_weak_ptr& e) {
 > +        isc_throw(isc::Unexpected, "Failed to update timer");
 >      }
 > }}}
 Yes, the exception will be thrown only in case an internal bug. I modified
 the code to cause assertion failure instead of throwing isc::Unexpected.

 > - And, although it's not for this branch, I'd suggest using more
 >   informative error, preferably convert the asio error to string:
 > {{{
 >      } catch (const asio::system_error& e) {
 >          isc_throw(isc::Unexpected, "Failed to update timer");
 > }}}
 >   Or, if it's not feasible, the unused 'e' should be removed.
 I modified the code to convert the error to string.

 > Finally, I think the technique using shared_from_this is useful in
 > other cases (specifically for the resolver code) and is uncommon.  So,
 > could you introduce it to the bind10-dev list so that everyone will be
 > aware of it?
 I posted an e-mail to bind10-dev list. I hope it will be helpful.

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