[FFmpeg-devel] ffmpeg: Ignore SIGPIPE

On systems which deliver SIGPIPE (Unices), a broken pipe will currently
result in the immediate termination of the ffmpeg process (the default
disposition as required by POSIX).  This is undesirable, because while
the broken pipe is likely fatal to useful cleanup of whatever component
is writing to it, there might be other components which can do useful
cleanup - for example, a muxer on another stream may still need to write
indexes to complete a file.  Therefore, set the signal disposition for
SIGPIPE to ignore the signal - the call which caused the signal will
fail with EPIPE and the error will be propagated upwards like any other
I/O failure on a single stream.
---
On 18/01/18 15:55, Mark Thompson wrote:
> On 18/01/18 14:49, Dave Rice wrote:
>> Thread bump.
>>
>>> On Jan 11, 2018, at 5:51 PM, Nicolas George <george@nsup.org> wrote:
>>>
>>> Moritz Barsnick (2018-01-11):
>>>> This patch doesn't change the handling of SIGTERM
>>>
>>> You should have read SIGPIPE, obviously.
>>>
>>>> Is SIGPIPE an interactive signal?
>>>
>>> Of course not.
>>>
>>>> 				    Anything on the other side of output
>>>> file(name) "-" or "pipe:N" may terminate for some reason.
>>>
>>> Yes, that is exactly what SIGPIPE is for.
>>>
>>>> This patch does NOT try to ignore anything. ffmpeg won't keep running
>>>> due to ignoring of SIGPIPE, it will terminate more cleanly due to
>>>> handling it. The former is not desired. (But yes, shall handing to
>>>> enforce ignoring it would allow that.)
>>>
>>> It will terminate less cleanly than if you do the right thing with
>>> SIGPIPE.
>>
>> This patch has been working for me and ffmpeg terminates cleanly with SIGPIPE with a valid output (moov atom written with mov or cues/seekhead written with mkv). Not sure if I understand well the disadvantage of this patch.
> 
> Making SIGPIPE have an effect like this seems highly dubious to me, but I admit that since it always killed the process before it feels like an improvement.
> 
> Changing it to ignore the signal (set it to SIG_IGN) instead sounds like a safer option?  I think that would move the code to having the same behaviour on Unices as on Windows, since Windows has no SIGPIPE.

To offer this suggestion in a more concrete form.


 fftools/ffmpeg.c | 3 +++
 1 file changed, 3 insertions(+)

2018-01-19 0:42 GMT+01:00 Mark Thompson <sw@jkqxz.net>:

> To offer this suggestion in a more concrete form.

Works as expected here except for printing an error which
may be intended.

Thank you, Carl Eugen

On 19/01/18 00:08, Carl Eugen Hoyos wrote:
> 2018-01-19 0:42 GMT+01:00 Mark Thompson <sw@jkqxz.net>:
> 
>> To offer this suggestion in a more concrete form.
> 
> Works as expected here except for printing an error which
> may be intended.

Yes, that's intended.  Data may have been lost unexpectedly (as with any I/O error), so I believe the user should be informed.

- Mark

Mark Thompson (2018-01-18):
> On systems which deliver SIGPIPE (Unices), a broken pipe will currently
> result in the immediate termination of the ffmpeg process (the default
> disposition as required by POSIX).  This is undesirable, because while
> the broken pipe is likely fatal to useful cleanup of whatever component
> is writing to it, there might be other components which can do useful
> cleanup - for example, a muxer on another stream may still need to write
> indexes to complete a file.  Therefore, set the signal disposition for
> SIGPIPE to ignore the signal - the call which caused the signal will
> fail with EPIPE and the error will be propagated upwards like any other
> I/O failure on a single stream.

I think it is a bad idea to make this unconditional. But I am tired to
fight uphill.

Regards,

On 18/01/18 23:42, Mark Thompson wrote:
> On systems which deliver SIGPIPE (Unices), a broken pipe will currently
> result in the immediate termination of the ffmpeg process (the default
> disposition as required by POSIX).  This is undesirable, because while
> the broken pipe is likely fatal to useful cleanup of whatever component
> is writing to it, there might be other components which can do useful
> cleanup - for example, a muxer on another stream may still need to write
> indexes to complete a file.  Therefore, set the signal disposition for
> SIGPIPE to ignore the signal - the call which caused the signal will
> fail with EPIPE and the error will be propagated upwards like any other
> I/O failure on a single stream.
> ---
>  fftools/ffmpeg.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 528849a2c6..918eb353aa 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -406,6 +406,9 @@ void term_init(void)
>  #ifdef SIGXCPU
>      signal(SIGXCPU, sigterm_handler);
>  #endif
> +#ifdef SIGPIPE
> +    signal(SIGPIPE, SIG_IGN); /* Broken pipe (POSIX). */
> +#endif
>  #if HAVE_SETCONSOLECTRLHANDLER
>      SetConsoleCtrlHandler((PHANDLER_ROUTINE) CtrlHandler, TRUE);
>  #endif
> 

I'll apply this tomorrow if there are no further comments.

Thanks,

- Mark

On 19/01/18 00:18, Mark Thompson wrote:
> On 19/01/18 00:08, Carl Eugen Hoyos wrote:
>> 2018-01-19 0:42 GMT+01:00 Mark Thompson <sw@jkqxz.net>:
>>
>>> To offer this suggestion in a more concrete form.
>>
>> Works as expected here except for printing an error which
>> may be intended.
> 
> Yes, that's intended.  Data may have been lost unexpectedly (as with any I/O error), so I believe the user should be informed.

And applied.

Thanks,

- Mark

Mark Thompson (2018-01-25):
> And applied.

I am glad to see that my advice was so well taken into account. From now
on, I will defer to you on matters relating to Unix system programming,
especially signals, shall I?

Regards,

On 25/01/18 23:22, Nicolas George wrote:
> Mark Thompson (2018-01-25):
>> And applied.
> 
> I am glad to see that my advice was so well taken into account. From now
> on, I will defer to you on matters relating to Unix system programming,
> especially signals, shall I?

Please don't.  In this case the semantics of ignoring the signal are clear and I think we can be confident the change is ok, but that certainly isn't true in almost all other cases (especially if people are going to catch signals and do something in the signal handler).

- Mark

Mark Thompson (2018-01-25):
> Please don't.  In this case the semantics of ignoring the signal are
> clear and I think we can be confident the change is ok, but that
> certainly isn't true in almost all other cases (especially if people
> are going to catch signals and do something in the signal handler).

Well, you pushed this patch disregarding my advice: obviously you
consider your grasp on signals better than mine. Good. We need somebody
that understands them.

Regards,