Change in pipe behavior between 3.4.1 and 3.4.2

[jasongdove]

Version

3.4.2

Details

I have some unit tests that use ffmpeg, and in the context of these tests the ffmpeg output is piped to null. With 3.4.1 everything works correctly and ffmpeg exits with code 0, but with 3.4.2 ffmpeg exits with code 1 and the following error:

av_interleaved_write_frame(): Broken pipe
Error writing trailer of pipe:1: Broken pipe
Error closing file pipe:1: Broken pipe

I tried looking through the changes between 3.4.1 and 3.4.2 but am not familiar enough with the project to identify the cause of the different behavior.

Could use some help determining if this is a regression or if the change in behavior is expected and I need to update my test code somehow.

Steps to reproduce

Repro project is at https://github.com/jasongdove/cliwrap-repro1 - ffmpeg is a required dependency.

The test will pass with CliWrap 3.4.1 and fail with CliWrap 3.4.2.

[Tyrrrz]

Thank you for the bug report.

I suspect it could be a result of this change: c55bba6

Can you try CliWrap@363997390859429c2a9ae4f28afa47157be56947 (previous commit)?

[jasongdove]

I pulled the repro project into the CliWrap solution and ran a git bisect, and the problematic commit is actually adca68a

Perhaps the change to NullPipeTarget.CopyFromAsync?

[Tyrrrz]

Interesting! Do you have any hunch on how this bug could be replicated without ffmpeg? I'd like to add a test for this but would prefer to avoid packaging ffmpeg in the repo.

[jasongdove]

I think the issue is that ffmpeg wants to write to the pipe (standard output) after it has been closed/is no longer being read from. It's less clear to me how that happens, but I'll do some experimentation with threading and flushing in a small standalone console app.

[jasongdove]

Am I reading this correctly that when using a null pipe target, the source stream (in my case standard output) is never read from?

CliWrap/CliWrap/PipeTarget.cs

Lines 142 to 148 in 82144b8

	internal class NullPipeTarget : PipeTarget
	{
	public override Task CopyFromAsync(Stream source, CancellationToken cancellationToken = default) =>
	!cancellationToken.IsCancellationRequested
	? Task.CompletedTask
	: Task.FromCanceled(cancellationToken);
	}

[Tyrrrz]

Yes, that's correct. That was the change you're pointing towards. I initially assumed that the streams would need to be exhausted to avoid deadlocks, but I could not produce an issue through my multiple attempts, so I replaced it with the current behavior. That's why I want to see if there's a simple test that reproduces the error you're having.

[Tyrrrz]

Looking at this answer on StackOverflow it does look like a bug in FFmpeg where it attempts to write to stdout that has already been closed. This issue does not seem to be reproducible with a .NET app writing to the console.

[jasongdove]

Perhaps I'm misunderstanding. Can you explain the purpose of this code after the change in question?

WithStandardOutputPipe(PipeTarget.Null)

Does it effectively do nothing? It seems to behave the same whether that line is present or absent.

[Tyrrrz]

Not exactly. What it does is that it immediately closes stdout/stderr upon starting the process. In all cases I tested, it resulted in the underlying process no longer writing the output and completing as expected.

Previous behavior was equivalent to WithStandardOutputPipe(PipeTarget.ToStream(Stream.Null)).

Does it effectively do nothing? It seems to behave the same whether that line is present or absent.

That's because it's the default value for that setting.

[jasongdove]

Okay, thanks for the explanation. I think the former behavior was more intuitive semantically (piping to the null stream rather than piping to nothing), but I can use the workaround that you posted to stay on the latest version.

You may also want to update the comment here

CliWrap/CliWrap/PipeTarget.cs

Lines 25 to 31 in aeb93a8

	public partial class PipeTarget
	{
	/// <summary>
	/// Pipe target that discards all data.
	/// Logical equivalent to <code>/dev/null</code>.
	/// </summary>
	public static PipeTarget Null { get; } = new NullPipeTarget();

[Tyrrrz]

Actually, after looking at your test more carefully, I understand what's happening now.

1. By default, CliWrap runs commands with stdin/stdout/stderr pipes closed (PipeSource.Null and PipeTarget.Null). This helps reduce memory allocation for data that is not consumed. Previously, the behavior of the default targets was equivalent to writing to a null-stream, where the data was read but immediately discarded. Now the data is not read, in fact it cannot even be written by the underlying process in the first place. In all cases I knew of, both old and new behaviors would be functionally equivalent, unless the process explicitly checks the existence of the pipe.
2. You are instructing FFmpeg to write the output of the operation to pipe:1 (i.e. stdout). Because that pipe is not open, it throws an error. This makes sense, because if you are explicitly specifying the output destination, you are probably interested in reading the data later, so the failure to write data is a reasonable cause for a fatal error. Note that this doesn't happen in other cases, for example if you're writing through FFmpeg to a file.

Because of this, I think it would make sense to explicitly pipe to Stream.Null in your case.

You may also want to update the comment here

Will do.

[jasongdove]

Yeah, for context the app is a video streaming server that normally pipes the output to clients. I didn't want to have separate logic for the tests, so piping to the null stream is desired.

Thanks for your help!