diff --git a/Assets/Tests/Editor/DynamicCodeToolTests/SharedRoslynCompilerWorkerHostTests.cs b/Assets/Tests/Editor/DynamicCodeToolTests/SharedRoslynCompilerWorkerHostTests.cs index e8355b815e..7f7e3a3c08 100644 --- a/Assets/Tests/Editor/DynamicCodeToolTests/SharedRoslynCompilerWorkerHostTests.cs +++ b/Assets/Tests/Editor/DynamicCodeToolTests/SharedRoslynCompilerWorkerHostTests.cs @@ -1,4 +1,5 @@ using System; +using System.ComponentModel; using System.Diagnostics; using System.IO; using System.Text; @@ -79,6 +80,198 @@ public void StartProcessLocked_WhenReplacingCachedProcess_ShouldDisposePreviousH } } + /// + /// Verifies a broken graceful shutdown channel still falls back to forced termination. + /// + [Test] + public void ExecuteProcessShutdown_WhenGracefulRequestThrowsIOException_ShouldStillForceKill() + { + IOException shutdownFailure = new("worker input closed"); + Exception loggedFailure = null; + int forceKillCallCount = 0; + int disposeCallCount = 0; + + SharedRoslynCompilerWorkerSession.ExecuteProcessShutdown( + hasExited: () => false, + requestGracefulShutdown: () => throw shutdownFailure, + forceKill: () => forceKillCallCount++, + dispose: () => disposeCallCount++, + logFailure: ex => loggedFailure = ex); + + Assert.That(loggedFailure, Is.SameAs(shutdownFailure)); + Assert.That(forceKillCallCount, Is.EqualTo(1)); + Assert.That(disposeCallCount, Is.EqualTo(1)); + } + + /// + /// Verifies a disposed graceful shutdown channel still falls back to forced termination. + /// + [Test] + public void ExecuteProcessShutdown_WhenGracefulRequestThrowsObjectDisposedException_ShouldStillForceKill() + { + ObjectDisposedException shutdownFailure = new("worker input"); + Exception loggedFailure = null; + int forceKillCallCount = 0; + int disposeCallCount = 0; + + SharedRoslynCompilerWorkerSession.ExecuteProcessShutdown( + hasExited: () => false, + requestGracefulShutdown: () => throw shutdownFailure, + forceKill: () => forceKillCallCount++, + dispose: () => disposeCallCount++, + logFailure: ex => loggedFailure = ex); + + Assert.That(loggedFailure, Is.SameAs(shutdownFailure)); + Assert.That(forceKillCallCount, Is.EqualTo(1)); + Assert.That(disposeCallCount, Is.EqualTo(1)); + } + + /// + /// Verifies an operating-system kill failure is logged without escaping process disposal. + /// + [Test] + public void ExecuteProcessShutdown_WhenForceKillThrowsWin32Exception_ShouldLogAndDispose() + { + Win32Exception shutdownFailure = new(5, "worker kill denied"); + Exception loggedFailure = null; + int disposeCallCount = 0; + + Assert.DoesNotThrow(() => SharedRoslynCompilerWorkerSession.ExecuteProcessShutdown( + hasExited: () => false, + requestGracefulShutdown: () => { }, + forceKill: () => throw shutdownFailure, + dispose: () => disposeCallCount++, + logFailure: ex => loggedFailure = ex)); + + Assert.That(loggedFailure, Is.SameAs(shutdownFailure)); + Assert.That(disposeCallCount, Is.EqualTo(1)); + } + + /// + /// Verifies an unknown exit state still attempts forced termination after logging the query failure. + /// + [Test] + public void ExecuteProcessShutdown_WhenForceExitQueryThrowsWin32Exception_ShouldStillForceKill() + { + Win32Exception queryFailure = new(5, "worker exit code unavailable"); + Exception loggedFailure = null; + int hasExitedCallCount = 0; + int forceKillCallCount = 0; + int disposeCallCount = 0; + int failureLogCount = 0; + + SharedRoslynCompilerWorkerSession.ExecuteProcessShutdown( + hasExited: () => + { + hasExitedCallCount++; + if (hasExitedCallCount == 2) + { + throw queryFailure; + } + + return false; + }, + requestGracefulShutdown: () => { }, + forceKill: () => forceKillCallCount++, + dispose: () => disposeCallCount++, + logFailure: ex => + { + failureLogCount++; + loggedFailure = ex; + }); + + Assert.That(loggedFailure, Is.SameAs(queryFailure)); + Assert.That(failureLogCount, Is.EqualTo(1)); + Assert.That(forceKillCallCount, Is.EqualTo(1)); + Assert.That(disposeCallCount, Is.EqualTo(1)); + } + + /// + /// Verifies a missing associated process skips forced termination after logging the query failure. + /// + [Test] + public void ExecuteProcessShutdown_WhenForceExitQueryThrowsInvalidOperationException_ShouldSkipForceKill() + { + InvalidOperationException queryFailure = new("worker process unavailable"); + Exception loggedFailure = null; + int hasExitedCallCount = 0; + int forceKillCallCount = 0; + int disposeCallCount = 0; + int failureLogCount = 0; + + SharedRoslynCompilerWorkerSession.ExecuteProcessShutdown( + hasExited: () => + { + hasExitedCallCount++; + if (hasExitedCallCount == 2) + { + throw queryFailure; + } + + return false; + }, + requestGracefulShutdown: () => { }, + forceKill: () => forceKillCallCount++, + dispose: () => disposeCallCount++, + logFailure: ex => + { + failureLogCount++; + loggedFailure = ex; + }); + + Assert.That(loggedFailure, Is.SameAs(queryFailure)); + Assert.That(failureLogCount, Is.EqualTo(1)); + Assert.That(forceKillCallCount, Is.Zero); + Assert.That(disposeCallCount, Is.EqualTo(1)); + } + + /// + /// Verifies successful graceful shutdown skips forced termination and disposes once. + /// + [Test] + public void ExecuteProcessShutdown_WhenGracefulRequestExitsProcess_ShouldSkipForceKill() + { + bool processExited = false; + int forceKillCallCount = 0; + int disposeCallCount = 0; + int failureLogCount = 0; + + SharedRoslynCompilerWorkerSession.ExecuteProcessShutdown( + hasExited: () => processExited, + requestGracefulShutdown: () => processExited = true, + forceKill: () => forceKillCallCount++, + dispose: () => disposeCallCount++, + logFailure: ex => failureLogCount++); + + Assert.That(forceKillCallCount, Is.Zero); + Assert.That(failureLogCount, Is.Zero); + Assert.That(disposeCallCount, Is.EqualTo(1)); + } + + /// + /// Verifies an already exited process skips both termination phases and disposes once. + /// + [Test] + public void ExecuteProcessShutdown_WhenProcessAlreadyExited_ShouldOnlyDispose() + { + int gracefulRequestCallCount = 0; + int forceKillCallCount = 0; + int disposeCallCount = 0; + int failureLogCount = 0; + + SharedRoslynCompilerWorkerSession.ExecuteProcessShutdown( + hasExited: () => true, + requestGracefulShutdown: () => gracefulRequestCallCount++, + forceKill: () => forceKillCallCount++, + dispose: () => disposeCallCount++, + logFailure: ex => failureLogCount++); + + Assert.That(gracefulRequestCallCount, Is.Zero); + Assert.That(forceKillCallCount, Is.Zero); + Assert.That(failureLogCount, Is.Zero); + Assert.That(disposeCallCount, Is.EqualTo(1)); + } + /// /// Verifies a pending compiler stream does not keep the bounded drain waiting. /// diff --git a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerSession.cs b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerSession.cs index 88a28b942e..da39886346 100644 --- a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerSession.cs +++ b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerSession.cs @@ -1,4 +1,5 @@ using System; +using System.ComponentModel; using System.Diagnostics; using System.IO; using System.Threading; @@ -104,37 +105,117 @@ internal void ShutdownProcessLocked() return; } - try - { - if (!workerProcess.HasExited) + ExecuteProcessShutdown( + hasExited: () => workerProcess.HasExited, + requestGracefulShutdown: () => { workerProcess.StandardInput.WriteLine( SharedRoslynCompilerWorkerProtocol.SharedCompilerWorkerQuitCommand); workerProcess.StandardInput.Flush(); workerProcess.WaitForExit(500); - } - - if (!workerProcess.HasExited) + }, + forceKill: () => { workerProcess.Kill(); workerProcess.WaitForExit(500); + }, + dispose: workerProcess.Dispose, + logFailure: LogWorkerShutdownFailure); + } + + internal static void ExecuteProcessShutdown( + Func hasExited, + Action requestGracefulShutdown, + Action forceKill, + Action dispose, + Action logFailure) + { + Debug.Assert(hasExited != null, "hasExited must not be null"); + Debug.Assert(requestGracefulShutdown != null, "requestGracefulShutdown must not be null"); + Debug.Assert(forceKill != null, "forceKill must not be null"); + Debug.Assert(dispose != null, "dispose must not be null"); + Debug.Assert(logFailure != null, "logFailure must not be null"); + + try + { + // A failed graceful request must not prevent the forced termination phase. + TryRequestGracefulShutdown(hasExited, requestGracefulShutdown, logFailure); + TryForceKill(hasExited, forceKill, logFailure); + } + finally + { + dispose(); + } + } + + private static void TryRequestGracefulShutdown( + Func hasExited, + Action requestGracefulShutdown, + Action logFailure) + { + try + { + if (!hasExited()) + { + requestGracefulShutdown(); } } catch (IOException ex) { - LogWorkerShutdownFailure(ex); + logFailure(ex); } catch (ObjectDisposedException ex) { - LogWorkerShutdownFailure(ex); + logFailure(ex); } catch (InvalidOperationException ex) { - LogWorkerShutdownFailure(ex); + logFailure(ex); } - finally + catch (Win32Exception ex) + { + logFailure(ex); + } + } + + private static void TryForceKill( + Func hasExited, + Action forceKill, + Action logFailure) + { + bool shouldForceKill; + try + { + shouldForceKill = !hasExited(); + } + catch (Win32Exception ex) + { + logFailure(ex); + // An unavailable exit code leaves process state unknown, so forced termination is safer. + shouldForceKill = true; + } + catch (InvalidOperationException ex) + { + logFailure(ex); + return; + } + + if (!shouldForceKill) + { + return; + } + + try + { + forceKill(); + } + catch (Win32Exception ex) + { + logFailure(ex); + } + catch (InvalidOperationException ex) { - workerProcess.Dispose(); + logFailure(ex); } }