On 02/20/2018 06:28 PM, 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.
>
The code from original pipe-source:
/* Hmm, nothing to do. Let's sleep */
pollfd->events = (short) (u->source->thread_info.state == PA_SOURCE_RUNNING ? POLLIN : 0);
if ((ret = pa_rtpoll_run(u->rtpoll)) < 0)
goto fail;
It just stop polling the pipe if source not running.
One difference - it will never get POLLHUP because pipe opened for both reading and writing.
--
Raman
