Keep a media process and its exit state usable by the caller that started it

MediaBrowser.MediaEncoding/Encoder/MediaEncoder.cs

@@ -1152,6 +1152,11 @@ namespace MediaBrowser.MediaEncoding.Encoder
                 {
                     process.Process.PriorityClass = ProcessPriorityClass.BelowNormal;
                 }
+                catch (InvalidOperationException)
+                {
+                    // The process finished before its priority could be lowered. That says nothing
+                    // about whether the platform allows it, so keep the capability for the next one.
+                }
                 catch (Exception ex)
                 {
                     _canSetProcessPriority = false;
@@ -1361,12 +1366,20 @@ namespace MediaBrowser.MediaEncoding.Encoder
             return _configurationManager.GetEncodingOptions().EnableSubtitleExtraction;
         }
 
-        private sealed class ProcessWrapper : IDisposable
+        internal sealed class ProcessWrapper : IDisposable
         {
             private readonly MediaEncoder _mediaEncoder;
 
+            // The exit event is raised on the thread pool, so it writes the state below while the
+            // caller that started the process is reading it.
+            private readonly Lock _exitLock = new();
+
             private bool _disposed = false;
 
+            private bool _hasExited;
+
+            private int? _exitCode;
+
             public ProcessWrapper(Process process, MediaEncoder mediaEncoder)
             {
                 Process = process;
@@ -1376,49 +1389,84 @@ namespace MediaBrowser.MediaEncoding.Encoder
 
             public Process Process { get; }
 
-            public bool HasExited { get; private set; }
+            // The exit event can lag behind the wait that returned, so ask the process rather than
+            // report one that has exited as still running.
+            public bool HasExited => ReadExitState().HasExited;
 
-            public int? ExitCode { get; private set; }
+            // As above: rather than report no exit code for a process that has one.
+            public int? ExitCode => ReadExitState().ExitCode;
+
+            private (bool HasExited, int? ExitCode) ReadExitState()
+            {
+                lock (_exitLock)
+                {
+                    if (!_hasExited && !_disposed)
+                    {
+                        try
+                        {
+                            if (Process.HasExited)
+                            {
+                                _hasExited = true;
+                                _exitCode = Process.ExitCode;
+                            }
+                        }
+                        catch (InvalidOperationException)
+                        {
+                            // No process is associated with this object, or it was disposed from
+                            // under us - ObjectDisposedException derives from this one.
+                        }
+                    }
+
+                    return (_hasExited, _exitCode);
+                }
+            }
 
             private void OnProcessExited(object sender, EventArgs e)
             {
                 var process = (Process)sender;
 
-                HasExited = true;
+                lock (_exitLock)
+                {
+                    _hasExited = true;
 
-                try
-                {
-                    ExitCode = process.ExitCode;
-                }
-                catch
-                {
+                    try
+                    {
+                        _exitCode = process.ExitCode;
+                    }
+                    catch
+                    {
+                    }
                 }
 
-                DisposeProcess(process);
+                // Only stop tracking it. The caller that started the process still holds it to read
+                // its output and its exit code, so disposing it here handed whoever was quickest to
+                // exit - an ffprobe on a file it rejects outright - an ObjectDisposedException.
+                Untrack();
             }
 
-            private void DisposeProcess(Process process)
+            private void Untrack()
             {
                 lock (_mediaEncoder._runningProcessesLock)
                 {
                     _mediaEncoder._runningProcesses.Remove(this);
                 }
-
-                process.Dispose();
             }
 
             public void Dispose()
             {
-                if (!_disposed)
+                lock (_exitLock)
                 {
-                    if (Process is not null)
+                    if (_disposed)
                     {
-                        Process.Exited -= OnProcessExited;
-                        DisposeProcess(Process);
+                        return;
                     }
+
+                    _disposed = true;
                 }
 
-                _disposed = true;
+                Process.Exited -= OnProcessExited;
+                Untrack();
+                Process.Dispose();
             }
         }
     }

tests/Jellyfin.MediaEncoding.Tests/Encoder/ProcessWrapperTests.cs

@@ -0,0 +1,103 @@
+using System;
+using System.Diagnostics;
+using System.Threading;
+using System.Threading.Tasks;
+using MediaBrowser.Controller.Configuration;
+using MediaBrowser.MediaEncoding.Encoder;
+using MediaBrowser.Model.Globalization;
+using MediaBrowser.Model.IO;
+using MediaBrowser.Model.MediaInfo;
+using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.Logging;
+using Moq;
+using Xunit;
+
+namespace Jellyfin.MediaEncoding.Tests.Encoder;
+
+public class ProcessWrapperTests
+{
+    [Fact]
+    public async Task ExitedProcess_StaysUsableForTheCallerThatStartedIt()
+    {
+        using var process = CreateProcess();
+        using var exitHandled = new ManualResetEventSlim(false);
+
+        using (var wrapper = new MediaEncoder.ProcessWrapper(process, CreateEncoder()))
+        {
+            // Subscribed after the wrapper, so by the time this is set the wrapper's own handler has
+            // already run: whatever it does to the process has happened.
+            process.Exited += (_, _) => exitHandled.Set();
+
+            process.Start();
+            await process.WaitForExitAsync(TestContext.Current.CancellationToken).ConfigureAwait(true);
+
+            Assert.True(exitHandled.Wait(TimeSpan.FromSeconds(15), TestContext.Current.CancellationToken), "The process never raised Exited.");
+
+            // The caller still owns the process here. Disposing it from the exit handler handed
+            // whoever exited quickest an ObjectDisposedException out of these three lines.
+            var output = await process.StandardOutput.ReadToEndAsync(TestContext.Current.CancellationToken).ConfigureAwait(true);
+            Assert.Equal("jellyfin", output.Trim());
+
+            Assert.True(wrapper.HasExited);
+            Assert.Equal(3, wrapper.ExitCode);
+        }
+    }
+
+    [Fact]
+    public async Task ExitState_IsReadableBeforeTheExitEventArrives()
+    {
+        using var process = CreateProcess();
+
+        using (var wrapper = new MediaEncoder.ProcessWrapper(process, CreateEncoder()))
+        {
+            process.Start();
+            await process.WaitForExitAsync(TestContext.Current.CancellationToken).ConfigureAwait(true);
+
+            // The exit event is raised on the thread pool and can lag behind the wait that just
+            // returned, so neither of these may depend on it having arrived.
+            Assert.True(wrapper.HasExited);
+            Assert.Equal(3, wrapper.ExitCode);
+        }
+    }
+
+    [Fact]
+    public async Task ExitCode_SurvivesDisposal()
+    {
+        using var process = CreateProcess();
+        var wrapper = new MediaEncoder.ProcessWrapper(process, CreateEncoder());
+
+        process.Start();
+        await process.WaitForExitAsync(TestContext.Current.CancellationToken).ConfigureAwait(true);
+
+        var exitCode = wrapper.ExitCode;
+        wrapper.Dispose();
+
+        Assert.Equal(exitCode, wrapper.ExitCode);
+        Assert.True(wrapper.HasExited);
+    }
+
+    private static MediaEncoder CreateEncoder()
+        => new(
+            Mock.Of<ILogger<MediaEncoder>>(),
+            Mock.Of<IServerConfigurationManager>(),
+            Mock.Of<IFileSystem>(),
+            Mock.Of<IBlurayExaminer>(),
+            Mock.Of<ILocalizationManager>(),
+            new ConfigurationBuilder().Build(),
+            Mock.Of<IServerConfigurationManager>());
+
+    // Writes to stdout and exits immediately with a non-zero code, standing in for the ffprobe that
+    // rejects a file outright - the process that used to win the race against its own caller.
+    private static Process CreateProcess()
+    {
+        var startInfo = OperatingSystem.IsWindows()
+            ? new ProcessStartInfo("cmd.exe", "/c echo jellyfin & exit 3")
+            : new ProcessStartInfo("/bin/sh", "-c \"printf 'jellyfin\\n'; exit 3\"");
+
+        startInfo.CreateNoWindow = true;
+        startInfo.UseShellExecute = false;
+        startInfo.RedirectStandardOutput = true;
+
+        return new Process { StartInfo = startInfo, EnableRaisingEvents = true };
+    }
+}