On 20.02.2018 17:08, Raman Shishniou wrote:
> On 02/20/2018 06:44 PM, Georg Chini wrote:
>> On 20.02.2018 16:28, Raman Shishniou wrote:
>>> On 02/20/2018 06:22 PM, Georg Chini wrote:
>>>> On 20.02.2018 16:13, Georg Chini wrote:
>>>>> On 20.02.2018 15:47, Raman Shishniou wrote:
>>>>>> On 02/20/2018 04:55 PM, Georg Chini wrote:
>>>>>>> On 20.02.2018 14:30, Raman Shishniou wrote:
>>>>>>>> On 02/20/2018 03:11 PM, Georg Chini wrote:
>>>>>>>>> On 19.02.2018 16:01, Raman Shyshniou wrote:
>>>>>>>>>> Currently the pipe-source will remain running even if no
>>>>>>>>>> writer is connected and therefore no data is produced.
>>>>>>>>>> This patch adds the autosuspend=<bool> option to prevent this.
>>>>>>>>>> Source will stay suspended if no writer is connected.
>>>>>>>>>> This option is enabled by default.
>>>>>>>>>> ---
>>>>>>>>>> src/modules/module-pipe-source.c | 279 ++++++++++++++++++++++++++++++---------
>>>>>>>>>> 1 file changed, 215 insertions(+), 64 deletions(-)
>>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> for (;;) {
>>>>>>>>>> int ret;
>>>>>>>>>> - struct pollfd *pollfd;
>>>>>>>>>> - pollfd = pa_rtpoll_item_get_pollfd(u->rtpoll_item, NULL);
>>>>>>>>>> + if (chunk.length) {
>>>>>>>>>> + /* We have a pending data, let's stop polling pipe.
>>>>>>>>>> + * Setting up pollfd->events = 0 is not enough to stop
>>>>>>>>>> + * POLLHUP spam if all writers are closed pipe.
>>>>>>>>>> + * We need to stop polling pipe completely */
>>>>>>>>>> + if (rtpoll_item) {
>>>>>>>>>> + pa_rtpoll_item_free(rtpoll_item);
>>>>>>>>>> + rtpoll_item = NULL;
>>>>>>>>>> + }
>>>>>>>>>> + } else {
>>>>>>>>>> + /* We have no pending data, let's start polling pipe */
>>>>>>>>>> + if (rtpoll_item == NULL) {
>>>>>>>>>> + rtpoll_item = pa_rtpoll_item_new(u->rtpoll, PA_RTPOLL_NEVER, 1);
>>>>>>>>>> + pollfd = pa_rtpoll_item_get_pollfd(rtpoll_item, NULL);
>>>>>>>>>> + pollfd->events = POLLIN;
>>>>>>>>>> + pollfd->fd = u->fd;
>>>>>>>>>> + }
>>>>>>>>>> + }
>>>>>>>>> Why do you need to do that? As in your previous patches you
>>>>>>>>> open the pipe for writing if all writers are disconnected, which
>>>>>>>>> should stop POLLHUP's.
>>>>>>>> This patch tries to be platform-independent.
>>>>>>>> Unfortunately not all platforms set POLLHUP when a writer closed pipe.
>>>>>>> Well, all platforms either set POLLIN or POLLHUP. If you have POLLHUP,
>>>>>>> you know the pipe was closed. If you have POLLIN and l = 0 on your
>>>>>>> read, you also know the pipe was closed.
>>>>>>> Then you open your corkfd and stop the POLLHUP/POLLIN.
>>>>>>> So why do you have to stop polling completely? When the next
>>>>>>> POLLIN comes, you know that data has been written and POLLHUP
>>>>>>> can no longer happen.
>>>>>>>
>>>>>> If I open pipe for writing before all data was read, i.e. when I see
>>>>>> POLLHUP - read() will never return 0. Also send suspend message when
>>>>>> I see POLLHUP is too early. I should open pipe for writing and send
>>>>>> suspend message when read() return 0.
>>>>>>
>>>>>>
>>>>>>>> Moreover, when POLLHUP was set there may be data in pipe that must be read
>>>>>>>> before suspend. Linux sets POLLIN in that case. Freebsd and macos sets
>>>>>>>> POLLIN|POLLHUP even if no any data in pipe, but no writers left. Some platforms
>>>>>>>> do not set POLLHUP at all. So do read() and check it return 0 must be the only
>>>>>>>> condition to suspend source and open pipe for writing.
>>>>>>> Yes, I agree, see above, but I still don't understand why you stop polling.
>>>>>>>
>>>>>>>> Also it cover cases when source suspended by user - we read data from pipe,
>>>>>>>> send resume, but source stays suspended. We must stop polling pipe, but
>>>>>>>> pollfd->events = 0 is not enough. If the writer make a open-write-close
>>>>>>>> while source stays suspended we will get endless POLLHUPs
>>>>>>> I think this could be covered differently. You can set a flag "is_suspended"
>>>>>>> when you have suspended the source because the writer disconnected.
>>>>>>> Then you can do something like
>>>>>>>
>>>>>>> if (SOURCE_IS_OPENED(source) || is_suspended) {
>>>>>>> if (!is_suspended && u->corkfd >= 0)
>>>>>>> close pipe for writing
>>>>>>>
>>>>>>> ...
>>>>>>>
>>>>>>> } else if (u->corkfd < 0)
>>>>>>> open pipe for writing
>>>>>>>
>>>>>>>
>>>>>> I need to stop polling to wait for source to be resumed if some bytes was
>>>>>> read and cannot be posted.
>>>>>>
>>>>>> I can get POLLHUP spam if source is suspended, some data is waiting for source
>>>>>> to be resumed and writer closed pipe.
>>>>>>
>>>>>> Example: source suspended by user and by autosuspend, pipe is waiting
>>>>>> for event (POLLIN), someone opened a pipe and send us data:
>>>>>>
>>>>>> First iteration:
>>>>>>
>>>>>> 1. poll:
>>>>>> events = POLLIN
>>>>>> pa_rtpoll_run()
>>>>>> revents == POLLIN
>>>>>> 2. read:
>>>>>> read() return 100 bytes
>>>>>> send message to resume source
>>>>>> 3. post:
>>>>>> source suspended, keep data in memchunk.
>>>>>>
>>>>>> Second iteration, 100 bytes waiting for source to be resumed:
>>>>>>
>>>>>> 1. poll:
>>>>>> events = 0
>>>>>> pa_rtpoll_run() (processed resume message, but source stays suspended)
>>>>>> revents == 0
>>>>>> 2. read:
>>>>>> some data still pending, do nothing
>>>>>> 3. post:
>>>>>> source still suspended, keep pending data
>>>>>>
>>>>>> ***writer closed pipe***
>>>>>>
>>>>>> Third iteration, POLLHUP spam, 100 bytes still waiting for source to be resumed:
>>>>> Why would you get POLLHUP spam here with the code I proposed above?
>>>>> Because the source is suspended by user but not suspended by you,
>>>>> the code would open the pipe for writing (and close it again as soon
>>>>> as the user suspend cause is removed). And on the first iteration, you
>>>>> can discard the data immediately when the source is not opened.
>>>> The way you are doing it now, the source will in fact keep running, even if it is
>>>> suspended by the user. One mistake above - you cannot immediately discard
>>>> the data because at that time you do not know if the source is user suspended
>>>> so you would have to discard any remaining data at some other point like that:
>>>>
>>>> if (SOURCE_IS_OPENED(source) || is_suspended) {
>>>> if (!is_suspended && u->corkfd >= 0) {
>>>> close pipe for writing
>>>> discard remaining data
>>>> }
>>>>
>>>> do all your other processing
>>>>
>>>> } else if (u->corkfd < 0)
>>>> open pipe for writing
>>>>
>>> The whole loop can be much easier if I can just discard user data :)
>>> But why I need to discard any data? I can just wait for source to be
>>> resumed temporary removing pipe from polling. The source will not keep
>>> running. It will just wait until it will be resumed.
>>>
>>> This is exactly what the original pipe-source is doing.
>>>
>> Not executing all that unnecessary code would be better. I never said that
>> the old code did the right thing.
>> Regarding discarding the data: If data was read while the source was suspended
>> by the user, it needs to be discarded on resume, because the the source is not
>> expected to have some old data.
>>
> I can just set pollfd->fd = -1 when no polling is needed instead of freeing
> the whole rtpoll_item. But I'm not sure if it's supported by any platform
> except of linux.
I thought about that as well but - like you - am not sure if this
is supported elsewhere.
>
> I think the old code is doing the right thing. If the source is suspended
> by user, it was done for a reason. And we don't know if the writer tries to
> prefill the pipe and than resume the source to avoid underruns or user just
> stopped the annoying sound. Anyway I think we shouldn't take into account
> the suspend cause. Even if it automatically suspended.
>
> --
The only suspend cause that would be treated special would be
the autosuspend cause and you can keep track of it easily. No need
to know any other reasons, it is enough to know that the source is
suspended.
