From d608026d177943411b36291a591ea1fddaa24ce9 Mon Sep 17 00:00:00 2001 From: Ashish <156739271+alahane-techtel@users.noreply.github.com> Date: Fri, 4 Jul 2025 23:12:05 +1000 Subject: [PATCH 1/8] Fixed race condition on Named pipe dispose/disconnect --- FFMpegCore/FFMpeg/Arguments/PipeArgument.cs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/FFMpegCore/FFMpeg/Arguments/PipeArgument.cs b/FFMpegCore/FFMpeg/Arguments/PipeArgument.cs index 0751c9e..0fa0f7c 100644 --- a/FFMpegCore/FFMpeg/Arguments/PipeArgument.cs +++ b/FFMpegCore/FFMpeg/Arguments/PipeArgument.cs @@ -31,8 +31,12 @@ namespace FFMpegCore.Arguments public void Post() { Debug.WriteLine($"Disposing NamedPipeServerStream on {GetType().Name}"); - Pipe?.Dispose(); - Pipe = null!; + lock(Pipe) + { + + Pipe?.Dispose(); + Pipe = null!; + } } public async Task During(CancellationToken cancellationToken = default) @@ -48,9 +52,15 @@ namespace FFMpegCore.Arguments finally { Debug.WriteLine($"Disconnecting NamedPipeServerStream on {GetType().Name}"); - if (Pipe is { IsConnected: true }) + lock (Pipe ?? new object()) + //if Pipe is null, then the lock doesnt matter, + //Because the next code will not execute anyways. + //so we can use a new object { - Pipe.Disconnect(); + if (Pipe is { IsConnected: true }) + { + Pipe.Disconnect(); + } } } } From 5f2147e20708a3475f32616bb34d8b684ed91987 Mon Sep 17 00:00:00 2001 From: Malte Rosenbjerg Date: Thu, 16 Oct 2025 13:03:26 +0200 Subject: [PATCH 2/8] Fix formatting --- FFMpegCore.Test/FFProbeTests.cs | 7 +- FFMpegCore.Test/VideoTest.cs | 4 +- FFMpegCore/FFMpeg/Arguments/PipeArgument.cs | 93 ++++++++++----------- 3 files changed, 52 insertions(+), 52 deletions(-) diff --git a/FFMpegCore.Test/FFProbeTests.cs b/FFMpegCore.Test/FFProbeTests.cs index 049fc2e..a702eed 100644 --- a/FFMpegCore.Test/FFProbeTests.cs +++ b/FFMpegCore.Test/FFProbeTests.cs @@ -5,6 +5,8 @@ namespace FFMpegCore.Test; [TestClass] public class FFProbeTests { + public TestContext TestContext { get; set; } + [TestMethod] public async Task Audio_FromStream_Duration() { @@ -97,7 +99,8 @@ public class FFProbeTests [Ignore("Consistently fails on GitHub Workflow ubuntu agents")] public async Task Uri_Duration() { - var fileAnalysis = await FFProbe.AnalyseAsync(new Uri("https://github.com/rosenbjerg/FFMpegCore/raw/master/FFMpegCore.Test/Resources/input_3sec.webm"), cancellationToken: TestContext.CancellationToken); + var fileAnalysis = await FFProbe.AnalyseAsync(new Uri("https://github.com/rosenbjerg/FFMpegCore/raw/master/FFMpegCore.Test/Resources/input_3sec.webm"), + cancellationToken: TestContext.CancellationToken); Assert.IsNotNull(fileAnalysis); } @@ -282,6 +285,4 @@ public class FFProbeTests var info = FFProbe.Analyse(TestResources.Mp4Video, customArguments: "-headers \"Hello: World\""); Assert.AreEqual(3, info.Duration.Seconds); } - - public TestContext TestContext { get; set; } } diff --git a/FFMpegCore.Test/VideoTest.cs b/FFMpegCore.Test/VideoTest.cs index dadb26e..9bcca7c 100644 --- a/FFMpegCore.Test/VideoTest.cs +++ b/FFMpegCore.Test/VideoTest.cs @@ -19,6 +19,8 @@ public class VideoTest { private const int BaseTimeoutMilliseconds = 15_000; + public TestContext TestContext { get; set; } + [TestMethod] [Timeout(BaseTimeoutMilliseconds, CooperativeCancellation = true)] public void Video_ToOGV() @@ -1072,6 +1074,4 @@ public class VideoTest Assert.AreEqual("h264", outputInfo.PrimaryVideoStream.CodecName); Assert.AreEqual("aac", outputInfo.PrimaryAudioStream!.CodecName); } - - public TestContext TestContext { get; set; } } diff --git a/FFMpegCore/FFMpeg/Arguments/PipeArgument.cs b/FFMpegCore/FFMpeg/Arguments/PipeArgument.cs index 0fa0f7c..ee2d6f7 100644 --- a/FFMpegCore/FFMpeg/Arguments/PipeArgument.cs +++ b/FFMpegCore/FFMpeg/Arguments/PipeArgument.cs @@ -2,70 +2,69 @@ using System.IO.Pipes; using FFMpegCore.Pipes; -namespace FFMpegCore.Arguments +namespace FFMpegCore.Arguments; + +public abstract class PipeArgument { - public abstract class PipeArgument + private readonly PipeDirection _direction; + + protected PipeArgument(PipeDirection direction) { - private string PipeName { get; } - public string PipePath => PipeHelpers.GetPipePath(PipeName); + PipeName = PipeHelpers.GetUnqiuePipeName(); + _direction = direction; + } - protected NamedPipeServerStream Pipe { get; private set; } = null!; - private readonly PipeDirection _direction; + private string PipeName { get; } + public string PipePath => PipeHelpers.GetPipePath(PipeName); - protected PipeArgument(PipeDirection direction) + protected NamedPipeServerStream Pipe { get; private set; } = null!; + public abstract string Text { get; } + + public void Pre() + { + if (Pipe != null) { - PipeName = PipeHelpers.GetUnqiuePipeName(); - _direction = direction; + throw new InvalidOperationException("Pipe already has been opened"); } - public void Pre() - { - if (Pipe != null) - { - throw new InvalidOperationException("Pipe already has been opened"); - } + Pipe = new NamedPipeServerStream(PipeName, _direction, 1, PipeTransmissionMode.Byte, PipeOptions.Asynchronous); + } - Pipe = new NamedPipeServerStream(PipeName, _direction, 1, PipeTransmissionMode.Byte, PipeOptions.Asynchronous); + public void Post() + { + Debug.WriteLine($"Disposing NamedPipeServerStream on {GetType().Name}"); + lock (Pipe) + { + Pipe?.Dispose(); + Pipe = null!; } + } - public void Post() + public async Task During(CancellationToken cancellationToken = default) + { + try { - Debug.WriteLine($"Disposing NamedPipeServerStream on {GetType().Name}"); - lock(Pipe) - { - - Pipe?.Dispose(); - Pipe = null!; - } + await ProcessDataAsync(cancellationToken).ConfigureAwait(false); } - - public async Task During(CancellationToken cancellationToken = default) + catch (OperationCanceledException) { - try + Debug.WriteLine($"ProcessDataAsync on {GetType().Name} cancelled"); + } + finally + { + Debug.WriteLine($"Disconnecting NamedPipeServerStream on {GetType().Name}"); + lock (Pipe ?? new object()) + //if Pipe is null, then the lock doesnt matter, + //Because the next code will not execute anyways. + //so we can use a new object { - await ProcessDataAsync(cancellationToken).ConfigureAwait(false); - } - catch (OperationCanceledException) - { - Debug.WriteLine($"ProcessDataAsync on {GetType().Name} cancelled"); - } - finally - { - Debug.WriteLine($"Disconnecting NamedPipeServerStream on {GetType().Name}"); - lock (Pipe ?? new object()) - //if Pipe is null, then the lock doesnt matter, - //Because the next code will not execute anyways. - //so we can use a new object + if (Pipe is { IsConnected: true }) { - if (Pipe is { IsConnected: true }) - { - Pipe.Disconnect(); - } + Pipe.Disconnect(); } } } - - protected abstract Task ProcessDataAsync(CancellationToken token); - public abstract string Text { get; } } + + protected abstract Task ProcessDataAsync(CancellationToken token); } From 6fc811bc39718bcb71c1b0a32a7a56c30cbcfaac Mon Sep 17 00:00:00 2001 From: Malte Rosenbjerg Date: Thu, 16 Oct 2025 13:04:58 +0200 Subject: [PATCH 3/8] Move comment --- FFMpegCore/FFMpeg/Arguments/PipeArgument.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/FFMpegCore/FFMpeg/Arguments/PipeArgument.cs b/FFMpegCore/FFMpeg/Arguments/PipeArgument.cs index ee2d6f7..3b03866 100644 --- a/FFMpegCore/FFMpeg/Arguments/PipeArgument.cs +++ b/FFMpegCore/FFMpeg/Arguments/PipeArgument.cs @@ -53,10 +53,11 @@ public abstract class PipeArgument finally { Debug.WriteLine($"Disconnecting NamedPipeServerStream on {GetType().Name}"); + + //if Pipe is null, then the lock doesnt matter, + //Because the next code will not execute anyways. + //so we can use a new object lock (Pipe ?? new object()) - //if Pipe is null, then the lock doesnt matter, - //Because the next code will not execute anyways. - //so we can use a new object { if (Pipe is { IsConnected: true }) { From 29f40b88af0a04b30752ce7b15daba546461bbe7 Mon Sep 17 00:00:00 2001 From: Malte Rosenbjerg Date: Thu, 16 Oct 2025 13:09:52 +0200 Subject: [PATCH 4/8] Use private readonly object for locking --- FFMpegCore/FFMpeg/Arguments/PipeArgument.cs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/FFMpegCore/FFMpeg/Arguments/PipeArgument.cs b/FFMpegCore/FFMpeg/Arguments/PipeArgument.cs index 3b03866..69fd990 100644 --- a/FFMpegCore/FFMpeg/Arguments/PipeArgument.cs +++ b/FFMpegCore/FFMpeg/Arguments/PipeArgument.cs @@ -7,10 +7,12 @@ namespace FFMpegCore.Arguments; public abstract class PipeArgument { private readonly PipeDirection _direction; + private readonly object _pipeLock; protected PipeArgument(PipeDirection direction) { PipeName = PipeHelpers.GetUnqiuePipeName(); + _pipeLock = new object(); _direction = direction; } @@ -33,7 +35,7 @@ public abstract class PipeArgument public void Post() { Debug.WriteLine($"Disposing NamedPipeServerStream on {GetType().Name}"); - lock (Pipe) + lock (_pipeLock) { Pipe?.Dispose(); Pipe = null!; @@ -53,11 +55,7 @@ public abstract class PipeArgument finally { Debug.WriteLine($"Disconnecting NamedPipeServerStream on {GetType().Name}"); - - //if Pipe is null, then the lock doesnt matter, - //Because the next code will not execute anyways. - //so we can use a new object - lock (Pipe ?? new object()) + lock (_pipeLock) { if (Pipe is { IsConnected: true }) { From 262c3f1b4f6668ad70cd92b322f7777973221cf5 Mon Sep 17 00:00:00 2001 From: Malte Rosenbjerg Date: Thu, 16 Oct 2025 13:10:33 +0200 Subject: [PATCH 5/8] Wrap remaining Pipe access in lock --- FFMpegCore/FFMpeg/Arguments/PipeArgument.cs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/FFMpegCore/FFMpeg/Arguments/PipeArgument.cs b/FFMpegCore/FFMpeg/Arguments/PipeArgument.cs index 69fd990..0e51947 100644 --- a/FFMpegCore/FFMpeg/Arguments/PipeArgument.cs +++ b/FFMpegCore/FFMpeg/Arguments/PipeArgument.cs @@ -24,12 +24,15 @@ public abstract class PipeArgument public void Pre() { - if (Pipe != null) + lock (_pipeLock) { - throw new InvalidOperationException("Pipe already has been opened"); - } + if (Pipe != null) + { + throw new InvalidOperationException("Pipe already has been opened"); + } - Pipe = new NamedPipeServerStream(PipeName, _direction, 1, PipeTransmissionMode.Byte, PipeOptions.Asynchronous); + Pipe = new NamedPipeServerStream(PipeName, _direction, 1, PipeTransmissionMode.Byte, PipeOptions.Asynchronous); + } } public void Post() From 2d149d4c9a03b311d014242215a7cf1a006f4bc6 Mon Sep 17 00:00:00 2001 From: Malte Rosenbjerg Date: Thu, 16 Oct 2025 13:52:17 +0200 Subject: [PATCH 6/8] Initialize lock inline --- FFMpegCore/FFMpeg/Arguments/PipeArgument.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/FFMpegCore/FFMpeg/Arguments/PipeArgument.cs b/FFMpegCore/FFMpeg/Arguments/PipeArgument.cs index 0e51947..4011c23 100644 --- a/FFMpegCore/FFMpeg/Arguments/PipeArgument.cs +++ b/FFMpegCore/FFMpeg/Arguments/PipeArgument.cs @@ -7,12 +7,11 @@ namespace FFMpegCore.Arguments; public abstract class PipeArgument { private readonly PipeDirection _direction; - private readonly object _pipeLock; + private readonly object _pipeLock = new(); protected PipeArgument(PipeDirection direction) { PipeName = PipeHelpers.GetUnqiuePipeName(); - _pipeLock = new object(); _direction = direction; } From 85e7170fd97d9aac1fc0c278d5264100c5c389bb Mon Sep 17 00:00:00 2001 From: Malte Rosenbjerg Date: Thu, 16 Oct 2025 14:04:10 +0200 Subject: [PATCH 7/8] Improve test readability --- FFMpegCore.Test/FFMpegArgumentProcessorTest.cs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/FFMpegCore.Test/FFMpegArgumentProcessorTest.cs b/FFMpegCore.Test/FFMpegArgumentProcessorTest.cs index 1820b9b..daf2d9a 100644 --- a/FFMpegCore.Test/FFMpegArgumentProcessorTest.cs +++ b/FFMpegCore.Test/FFMpegArgumentProcessorTest.cs @@ -15,7 +15,7 @@ public class FFMpegArgumentProcessorTest [TestMethod] public void Processor_GlobalOptions_GetUsed() { - var globalWorkingDir = "Whatever"; + var globalWorkingDir = "Whatever1"; var processor = CreateArgumentProcessor(); try @@ -47,7 +47,7 @@ public class FFMpegArgumentProcessorTest [TestMethod] public void Processor_Options_CanBeOverridden_And_Configured() { - var globalConfig = "Whatever"; + var globalConfig = "Whatever2"; try { @@ -59,14 +59,14 @@ public class FFMpegArgumentProcessorTest var overrideOptions = new FFOptions { WorkingDirectory = "override" }; GlobalFFOptions.Configure(new FFOptions { WorkingDirectory = globalConfig, TemporaryFilesFolder = globalConfig, BinaryFolder = globalConfig }); - var options = processor.GetConfiguredOptions(overrideOptions); + var configuredOptions = processor.GetConfiguredOptions(overrideOptions); - Assert.AreEqual(options.WorkingDirectory, overrideOptions.WorkingDirectory); - Assert.AreEqual(options.TemporaryFilesFolder, overrideOptions.TemporaryFilesFolder); - Assert.AreEqual(options.BinaryFolder, overrideOptions.BinaryFolder); + Assert.AreEqual(configuredOptions.WorkingDirectory, overrideOptions.WorkingDirectory); + Assert.AreEqual(configuredOptions.TemporaryFilesFolder, overrideOptions.TemporaryFilesFolder); + Assert.AreEqual(configuredOptions.BinaryFolder, overrideOptions.BinaryFolder); - Assert.AreEqual(sessionTempDir, options.TemporaryFilesFolder); - Assert.AreNotEqual(globalConfig, options.BinaryFolder); + Assert.AreEqual(sessionTempDir, configuredOptions.TemporaryFilesFolder); + Assert.AreNotEqual(globalConfig, configuredOptions.BinaryFolder); } finally { @@ -77,7 +77,7 @@ public class FFMpegArgumentProcessorTest [TestMethod] public void Options_Global_And_Session_Options_Can_Differ() { - var globalWorkingDir = "Whatever"; + var globalWorkingDir = "Whatever3"; try { From 7c71a70a0cb0a3b1a5122ba3c026bbe56c5e3506 Mon Sep 17 00:00:00 2001 From: Malte Rosenbjerg Date: Thu, 16 Oct 2025 14:26:53 +0200 Subject: [PATCH 8/8] Revert "Improve test readability" This reverts commit 85e7170fd97d9aac1fc0c278d5264100c5c389bb. --- FFMpegCore.Test/FFMpegArgumentProcessorTest.cs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/FFMpegCore.Test/FFMpegArgumentProcessorTest.cs b/FFMpegCore.Test/FFMpegArgumentProcessorTest.cs index daf2d9a..1820b9b 100644 --- a/FFMpegCore.Test/FFMpegArgumentProcessorTest.cs +++ b/FFMpegCore.Test/FFMpegArgumentProcessorTest.cs @@ -15,7 +15,7 @@ public class FFMpegArgumentProcessorTest [TestMethod] public void Processor_GlobalOptions_GetUsed() { - var globalWorkingDir = "Whatever1"; + var globalWorkingDir = "Whatever"; var processor = CreateArgumentProcessor(); try @@ -47,7 +47,7 @@ public class FFMpegArgumentProcessorTest [TestMethod] public void Processor_Options_CanBeOverridden_And_Configured() { - var globalConfig = "Whatever2"; + var globalConfig = "Whatever"; try { @@ -59,14 +59,14 @@ public class FFMpegArgumentProcessorTest var overrideOptions = new FFOptions { WorkingDirectory = "override" }; GlobalFFOptions.Configure(new FFOptions { WorkingDirectory = globalConfig, TemporaryFilesFolder = globalConfig, BinaryFolder = globalConfig }); - var configuredOptions = processor.GetConfiguredOptions(overrideOptions); + var options = processor.GetConfiguredOptions(overrideOptions); - Assert.AreEqual(configuredOptions.WorkingDirectory, overrideOptions.WorkingDirectory); - Assert.AreEqual(configuredOptions.TemporaryFilesFolder, overrideOptions.TemporaryFilesFolder); - Assert.AreEqual(configuredOptions.BinaryFolder, overrideOptions.BinaryFolder); + Assert.AreEqual(options.WorkingDirectory, overrideOptions.WorkingDirectory); + Assert.AreEqual(options.TemporaryFilesFolder, overrideOptions.TemporaryFilesFolder); + Assert.AreEqual(options.BinaryFolder, overrideOptions.BinaryFolder); - Assert.AreEqual(sessionTempDir, configuredOptions.TemporaryFilesFolder); - Assert.AreNotEqual(globalConfig, configuredOptions.BinaryFolder); + Assert.AreEqual(sessionTempDir, options.TemporaryFilesFolder); + Assert.AreNotEqual(globalConfig, options.BinaryFolder); } finally { @@ -77,7 +77,7 @@ public class FFMpegArgumentProcessorTest [TestMethod] public void Options_Global_And_Session_Options_Can_Differ() { - var globalWorkingDir = "Whatever3"; + var globalWorkingDir = "Whatever"; try {