[pulseaudio-discuss] [PATCH 2/2] modules, pulse, core: React to failing pa_memblockq_push calls.

[David Henningsson]

On 05/24/2012 09:38 AM, Jarkko Suontausta wrote:
> This adds an assertion to pa_memblockq_push() and pa_memblockq_push_align()
> calls, the return value of which was mostly ignored in the core and modules.
> The calls return a negative value in case the maximum memblock queue size
> would be exceeded.
>
> The maximum queue sizes are currently set to be at least 16 megabytes, which
> corresponds to about 22 seconds of 16-bit 8-channel audio at 48 kHz, or well
> over a minute of stereo audio. If the queue gets full, it should be a sign
> of an internal error.

Just to point out, this assumption is not strictly true: maxlength for 
some buffers are very settable by the client, although most clients do 
not, I believe it's a way for the client to prefer underruns over higher 
latencies.

Hopefully all those buffers fall under the FIXME conditions below? 
Remember, there are flags that make the client buffers affect other 
buffers as well - PA_STREAM_ADJUST_LATENCY comes to mind, but there 
might be more.

> In some cases, mostly when the output memblock queue gets full due to
> external circumstances, asserting is not feasible. Those parts of the
> code are marked with a FIXME comment instead.

Also, we don't want to crash PulseAudio on broken/misbehaving clients. 
Is there a risk that if the client reads or writes half a sample, that 
the first patch will cause PulseAudio to crash?



I've talked about over-usage of assertions earlier, and maybe this is 
one of those cases. Sometimes it seems we accept a work flow as follows:

1) We discover that the return value is not checked, and after various 
degrees of thinking it through, we add an assertion because we can't 
figure out how it could fail without things being very utterly broken 
somewhere else.

2) We discover that PulseAudio starts to crash due to the added 
assertion, we figure out why this actually is a proper use case for 
failure and add proper error handling.

3) Everything is great, hooray!

The problem is that for the end user, the assertion is a worse 
experience than the unchecked return value. I feel responsible for 
shipping a stable PulseAudio to millions of Ubuntu users, so even if 
something just happens once in a million, I'm going to end up seeing bug 
reports for it. Also, the desktop space is more diverse than the mobile 
space (that I'm assuming Digia works in), so we're more likely to have 
misbehaving clients, weird module setups, and so on.

And from there to figuring out why the assertion fails, what the proper 
patch etc is, that's complex and time intensive for me, especially as I 
usually can not reproduce the error myself - at best, I get a stack 
trace to work with.

Combine this with the fact that you say you don't have time to fix the 
FIXMEs (who do you expect to do that?), and I become worried that the 
level of thinking through before adding these assertions is not high 
enough. Hopefully Tanu has done more thinking.

I don't want to block stuff, or be a moaner, but I can't help to wonder 
if this patch is more likely to improve things or regress them... :-/

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic