[asterisk-dev] [Code Review] Revert astobj2 change, add unit test for regression

[David Vossel]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1254/#review3669
-----------------------------------------------------------



/trunk/tests/test_astobj2.c
<https://reviewboard.asterisk.org/r/1254/#comment7367>

    unref obj on iteration



/trunk/tests/test_astobj2.c
<https://reviewboard.asterisk.org/r/1254/#comment7366>

    unref obj on iteration



/trunk/tests/test_astobj2.c
<https://reviewboard.asterisk.org/r/1254/#comment7365>

    obj isn't unreffed here.


- David


On 2011-06-02 18:46:56, Russell Bryant wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1254/
> -----------------------------------------------------------
> 
> (Updated 2011-06-02 18:46:56)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> The astobj2 unit tests have been failing in trunk a few times lately.  I still can't explain the failures, but while reviewing recent changes and trying to think of what could cause a problem, I thought of a case where a recent optimization will break things.  Specifically, this commit:
> 
> 
> ------------------------------------------------------------------------
> r316962 | schmidts | 2011-05-05 02:09:20 -0500 (Thu, 05 May 2011) | 10 lines
> 
> Adding the Move to Front Hash functionality
> 
> Moving a found object to the front of its bucket to reduce the necessary traversal steps to find an object. This change improves the search time on large system with many data or in link lists.
> 
> (closes issue #19233)
> Reported by: schmidts
> 
> Review: https://reviewboard.asterisk.org/r/1201/
> ------------------------------------------------------------------------
> 
> The complication with this has to do with how astobj2 iterators work.  The iterators are implemented in such a way that you do not have to hold the lock on the container for the entire duration of the container.  The lock is only held while getting the next element.  This works by using some internal version numbers to detect changes to the container while the lock was not held.  As a part of this code, it assumes that the order of elements in a given hash bucket are _always_ ordered by how long they have been in the container.
> 
> 
> Diffs
> -----
> 
>   /trunk/main/astobj2.c 321657 
>   /trunk/tests/test_astobj2.c 321657 
> 
> Diff: https://reviewboard.asterisk.org/r/1254/diff
> 
> 
> Testing
> -------
> 
> The included new unit test demonstrates the regression.  This test passes in Asterisk 1.8 (where this commit doesn't exist).  The test fails in trunk with this message:
> 
> 
> START  /main/astobj2/ - astobj2_test2 
> [test_astobj2.c:astobj2_test_2:481]: iterate take 3, expected '5', only saw '4' objects
> END    /main/astobj2/ - astobj2_test2 Time: <1ms Result: FAIL
> 
> After reverting the changes to main/astobj2.c, this test passes in trunk.
> 
> 
> Thanks,
> 
> Russell
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110603/f8ebe6b3/attachment-0001.htm>