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

[Mark Michelson]

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

Ship it!


It looks fine by me, and I say good riddance to the embedded frame in the filestream.

Of course, the only other thing I'd recommend is to get rid of all astobj2 usage in file.c since filestreams gain no benefit from being refcounted.

- Mark


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
> 
>