[pulseaudio-discuss] Releasing stream resources when the client receives SIGSTOP

[David Henningsson]

On 2014-11-24 14:18, Tanu Kaskinen wrote:
> On Mon, 2014-11-24 at 13:10 +0100, David Henningsson wrote:
>>
>> On 2014-11-24 12:04, Tanu Kaskinen wrote:
>>> On Mon, 2014-11-24 at 11:12 +0100, David Henningsson wrote:
>>>>
>>>> On 2014-11-20 04:44, Ricardo Salveti wrote:
>>>>> Hey,
>>>>>
>>>>> When investigating the issue we had on Ubuntu Touch, described by bug
>>>>> https://bugs.launchpad.net/ubuntu-rtm/+source/pulseaudio/+bug/1391230,
>>>>> I noticed that PulseAudio never releases the resources used by an
>>>>> active stream if the app gets a SIGSTOP, keeping pulse busy and
>>>>> consuming cpu until the app resumes or is closed by the user.
>>>>>
>>>>> On Ubuntu Touch that happens when the application is active playing
>>>>> audio/video, and the user moves back to the home scopes (Ubuntu Touch
>>>>> lifecycle will automatically send a SIGSTOP after 5 seconds). When
>>>>> checking that on my desktop, I also noticed that the same happens (by
>>>>> forcing a SIGSTOP against mplayer, for example). Pulse only releases
>>>>> the stream when the app pauses the stream, not necessarily when the
>>>>> app stops after receiving the signal.
>>>>>
>>>>> I raised this first with David to understand if it was indeed a valid
>>>>> use case, and he said that it was indeed something that it was
>>>>> probably never really considered
>>>>> (https://bugs.launchpad.net/ubuntu-rtm/+source/pulseaudio/+bug/1391230/comments/8).
>>>>>
>>>>> So before going and trying to deep dive and find a fix for the issue,
>>>>> I first wanted to understand from you guys if this is indeed a valid
>>>>> issue and what would be the best way to get this fixed. I know we're
>>>>> still using Pulse 4.0 on Ubuntu, but wanted to make sure to get
>>>>> something that would also be compatible with upstream.
>>>>
>>>> So when I discussed this with Ricardo, my suggestion was that we either
>>>>
>>>>     1) (ab)use the cork mechanism to stop/resume the client when we
>>>> discover that the client is SIGSTOPped, or
>>>
>>> This implies that we implement server-side corking, because currently
>>> the cork status is controlled by the client, and this "stalled" corking
>>> would be controlled by the server.
>>>
>>> Even though I have mentioned the "server-side corking" feature before
>>> (as something that we need and use in Tizen), I might not have explained
>>> how I think it should be implemented (Tizen's current implementation is
>>> less flexible than what I'd like). We should have a hashmap of "cork
>>> requests" in sink inputs and source outputs. The stream would be corked
>>> as long as the hashmap isn't empty, i.e. as long as someone thinks that
>>> the stream should be corked. The hashmap keys would be strings
>>> identifying the "corking reasons", so policy modules could easily add
>>> new reasons. The normal client-initiated corking would be one corking
>>> reason, and this stall detection would be another reason. Tizen's policy
>>> module would implement a third reason.
>>
>> +1 in general for this idea, to consolidate the "stall" concept with the
>> "server side cork" concept; the fewer concepts on the more use cases,
>> the better :-)
>>
>> I wonder what we should do with the "cork requests" though. I feel under
>> this scheme, we should deprecate them and replace them with a "you have
>> been (un)corked" message, possibly including the different reasons as
>> strings, so the client can determine whether the cork happened because
>> the client requested so, or if it was for some other reason.
>
> I'd vote for not removing the old cork request feature, because there
> are applications relying it. If the feature is seen as unnecessary for
> new/updated applications, I'm ok with marking it as deprecated in the
> documentation.
>
> As for sending some new kind of message like "you've been corked", I
> don't think that's necessary. If the introspection API allows pactl to
> show the cork status, including corking done by the server (possibly
> also including the reason string(s)), then the introspection API should
> be sufficient also for applications that need to get notified when their
> own stream gets corked by the server.

And if the reasons change, that gives an event/notification on the stream?

> Talking about the client API, there's the question that how should
> pa_sink_input_info.corked and pa_stream_is_corked(). For backwards
> compatibility reasons, I think at least pa_stream_is_corked() should
> return 1 only if the stream is corked by the application, and possibly
> it would be good to apply the same policy for pa_sink_input_info.corked
> too (but I think changing the pa_sink_input_info.corked semantics is
> less likely to cause breakage).
>
> Hmm... I realized there's another problem with stopped clients: not only
> do their streaming get stalled, but they also stop reading events that
> they may be subscribed to. The code looks like the daemon will buffer
> data until it runs out of memory, which seems like a bad idea...
>
> Let's assume that there's a simple application that just subscribes to
> events and does nothing else ("pactl subscribe" would fit the
> description). What to do when such application gets stopped? How do we
> notice that it's stopped, if it's not streaming? Should we send a ping
> message periodically? Or should we keep track of the amount of queued
> data, and if that reaches some threshold, then we consider that client
> stopped? Once we have declared the client unresponsive, should we
> disconnect it, or just drop subscription events until it becomes
> responsive again? In the latter case I think we should notify the
> application that events have been dropped.

I think most apps would like to receive buffered events once they are 
unstopped, so that should be the best for shorter stops.

But I also think it would be better to disconnect the client in case of 
overflow, rather than dropping events. The advanced clients should be 
able to deal with disconnect/reconnect and would then re-read the 
current state after reconnection.

Or we could generalise this: by dealing with this on the 
iochannel/srbchannel level, i e, we could disconnect the client in case 
the pstream.send_queue gets too large? That should at least protect the 
PulseAudio server from OOM, both for notifications and recording streams.

If the send_queue has more than, say, 100 [1] messages, we start a timer 
for 30 [1] seconds. If the send_queue is empty, we stop the timer. If 
the timer expires, the client is disconnected. How about that?

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

[1] Subject to bikeshedding