[asterisk-dev] [Code Review] Don't return an embedded frame from a filestream

[Kevin Fleming]

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

Ship it!


Well, other than you beating me to the punch (so now I can throw away the note on my desk to work on this <G>) this is fabulous. Go forth and conquer.

- Kevin


On 2009-09-30 13:53:41, Russell Bryant wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/386/
> -----------------------------------------------------------
> 
> (Updated 2009-09-30 13:53:41)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch is related to a number of issues on the bug tracker that show crashes related to freeing frames that came from a filestream.  A number of fixes have been made over time while trying to figure out these problems, but a number of people are still seeing the crash.  (Note that some of these bug reports include information about other problems.  I am specifically addressing the filestream frame crash here.)
> 
> I'm still not clear on what the exact problem is.  However, what is _very_ clear is that we have seen quite a few problems over time related to unexpected behavior when we try to use embedded frames as an optimization.  In some cases, this optimization doesn't really provide much due to improvements made in other areas.
> 
> In this case, the patch modifies filestream handling such that the embedded frame will not be returned.  ast_frisolate() is used to ensure that we end up with a completely mallocd frame.  In reality, though, we will not actually have to malloc every time.  For filestreams, the frame will almost always be allocated and freed in the same thread.  That means that the thread local frame cache will be used.  So, going this route doesn't hurt.
> 
> With this patch in place, some people have reported success in not seeing the crash anymore.  So, I propose that we merge it.
> 
> 
> This addresses bug 15817.
>     https://issues.asterisk.org/view.php?id=15817
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/include/asterisk/file.h 221299 
>   /branches/1.4/include/asterisk/frame.h 221299 
>   /branches/1.4/main/file.c 221299 
>   /branches/1.4/main/frame.c 221299 
> 
> Diff: https://reviewboard.asterisk.org/r/386/diff
> 
> 
> Testing
> -------
> 
> I have done some basic calls playing back files.  Users on the bug tracker have deployed it and reported success.
> 
> 
> Thanks,
> 
> Russell
> 
>