[asterisk-dev] [Code Review] 2746: Stasis: address refcount races; implementation comments

[opticron]

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



/trunk/include/asterisk/astobj2.h
<https://reviewboard.asterisk.org/r/2746/#comment18433>

    This is a useful macro to have, but I think it needs a name that differentiates it from ao2_ref much as ao2_cleanup differentiates itself from ao2_ref. Maybe ao2_bump? Since this would be breaking with the ao2_ref naming, NULL-safety would also be nice.



/trunk/main/stasis.c
<https://reviewboard.asterisk.org/r/2746/#comment18434>

    One letter variables are bad. How about forward_topic?


- opticron


On Aug. 7, 2013, 10:37 a.m., David Lee wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2746/
> -----------------------------------------------------------
> 
> (Updated Aug. 7, 2013, 10:37 a.m.)
> 
> 
> Review request for Asterisk Developers and Matt Jordan.
> 
> 
> Bugs: ASTERISK-22243
>     https://issues.asterisk.org/jira/browse/ASTERISK-22243
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Change r395954 reordered some stasis object destruction, which should
> have been fine. Unfortunately, it caused some hard to reproduce issues
> related to objects being accessed after they had been destroyed. The
> patch in r396329 fixed the destruction order problem; this patch
> addresses the underlying issue. A few other stasis-related fixes were
> also added.
> 
>  * Add ref-bumps around areas where objects may get transitively
>    destroyed. (For example, where we lock a topic, unref a subscription,
>    which unrefs the topic, which explodes the topic when we try to
>    unlock it.)
> 
>  * Wrote an extensive doxygen page about Stasis implementation,
>    relationships between objects, lifecycles of objects, how the
>    refcounting works, etc. Many other comments were added, corrected, or
>    cleaned up.
> 
>  * Added an assert to the topic dtor to catch extra ref decrements.
> 
>  * Fixed type used after destruction errors for graceful shutdown in
>    stasis_channels.c.
> 
>  * I added two unit tests in an attempt to catch destruction order
>    issues. Since the underlying cause is a race condition, though, the
>    tests rarely failed even when the code was wrong.
> 
>  * Fixed a leak in stasis_cache_pattern.c.
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/astobj2.h 396357 
>   /trunk/main/stasis.c 396357 
>   /trunk/main/stasis_cache.c 396357 
>   /trunk/main/stasis_cache_pattern.c 396357 
>   /trunk/main/stasis_channels.c 396357 
>   /trunk/tests/test_stasis.c 396357 
> 
> Diff: https://reviewboard.asterisk.org/r/2746/diff/
> 
> 
> Testing
> -------
> 
> Unit tests pass.
> 
> Reverted r396329, ran unit tests and scenarios that seemed to aggravate the race
> condition without error.
> 
> 
> Thanks,
> 
> David Lee
> 
>

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