diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs index ef8d0ff21a9f1..e3e764942c863 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs @@ -4,6 +4,7 @@ using System.Diagnostics; using System.IO; using System.Threading; +using System.Threading.Tasks; using Microsoft.CodeAnalysis.ErrorReporting; using Roslyn.Utilities; @@ -16,12 +17,13 @@ internal sealed class RemoteService public readonly Process Process; public readonly Service Service; private readonly int _processId; + private SemaphoreSlim _disposeSemaphore = new SemaphoreSlim(initialCount: 1); // output pumping threads (stream output from stdout/stderr of the host process to the output/errorOutput writers) private Thread _readOutputThread; // nulled on dispose private Thread _readErrorOutputThread; // nulled on dispose private InteractiveHost _host; // nulled on dispose - private bool _disposing; // set to true on dispose + private volatile ProcessExitHandlerStatus _processExitHandlerStatus; // set to Handled on dispose internal RemoteService(InteractiveHost host, Process process, int processId, Service service) { @@ -30,10 +32,10 @@ internal RemoteService(InteractiveHost host, Process process, int processId, Ser Debug.Assert(service != null); _host = host; - _disposing = false; this.Process = process; _processId = processId; this.Service = service; + _processExitHandlerStatus = ProcessExitHandlerStatus.Uninitialized; // TODO (tomat): consider using single-thread async readers _readOutputThread = new Thread(() => ReadOutput(error: false)); @@ -49,35 +51,36 @@ internal RemoteService(InteractiveHost host, Process process, int processId, Ser internal void HookAutoRestartEvent() { - const int ProcessExitHooked = 1; - const int ProcessExitHandled = 2; - - int processExitHandling = 0; + using (_disposeSemaphore.DisposableWait()) + { + // hook the event only once per process: + if (_processExitHandlerStatus == ProcessExitHandlerStatus.Uninitialized) + { + Process.Exited += ProcessExitedHandler; + _processExitHandlerStatus = ProcessExitHandlerStatus.Hooked; + } + } + } - async void localHandler(object _, EventArgs __) + private async void ProcessExitedHandler(object _, EventArgs __) + { + try { - try + using (await _disposeSemaphore.DisposableWaitAsync().ConfigureAwait(false)) { - if (Interlocked.Exchange(ref processExitHandling, ProcessExitHandled) == ProcessExitHooked) + if (_processExitHandlerStatus == ProcessExitHandlerStatus.Hooked) { - Process.Exited -= localHandler; - - if (!_disposing) - { - await _host.OnProcessExited(Process).ConfigureAwait(false); - } + Process.Exited -= ProcessExitedHandler; + _processExitHandlerStatus = ProcessExitHandlerStatus.Handled; + // Should set _processExitHandlerStatus before calling OnProcessExited to avoid deadlocks. + // Calling the host should be within the lock to prevent its disposing during the execution. + await _host.OnProcessExited(Process).ConfigureAwait(false); } } - catch (Exception e) when (FatalError.Report(e)) - { - throw ExceptionUtilities.Unreachable; - } } - - // hook the event only once per process: - if (Interlocked.Exchange(ref processExitHandling, ProcessExitHooked) == 0) + catch (Exception e) when (FatalError.Report(e)) { - Process.Exited += localHandler; + throw ExceptionUtilities.Unreachable; } } @@ -113,37 +116,38 @@ private void ReadOutput(bool error) internal void Dispose(bool joinThreads) { - // set _disposing so that we don't attempt restart the host anymore: - _disposing = true; - - InitiateTermination(Process, _processId); - - // only tests require joining the threads, so we can wait synchronously - if (joinThreads) + // There can be a call from host initiated from OnProcessExit. + // This check on the beginning helps to avoid a reentrancy. + if (_processExitHandlerStatus == ProcessExitHandlerStatus.Hooked) { - if (_readOutputThread != null) + using (_disposeSemaphore.DisposableWait()) { - try + if (_processExitHandlerStatus == ProcessExitHandlerStatus.Hooked) { - _readOutputThread.Join(); - } - catch (ThreadStateException) - { - // thread hasn't started + Process.Exited -= ProcessExitedHandler; + _processExitHandlerStatus = ProcessExitHandlerStatus.Handled; } } + } - if (_readErrorOutputThread != null) - { - try - { - _readErrorOutputThread.Join(); - } - catch (ThreadStateException) - { - // thread hasn't started - } - } + InitiateTermination(Process, _processId); + + try + { + _readOutputThread?.Join(); + } + catch (ThreadStateException) + { + // thread hasn't started + } + + try + { + _readErrorOutputThread?.Join(); + } + catch (ThreadStateException) + { + // thread hasn't started } // null the host so that we don't attempt to write to the buffer anymore: @@ -166,6 +170,13 @@ internal static void InitiateTermination(Process process, int processId) Debug.WriteLine("InteractiveHostProcess: can't terminate process {0}: {1}", processId, e.Message); } } + + private enum ProcessExitHandlerStatus + { + Uninitialized = 0, + Hooked = 1, + Handled = 2 + } } } } diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.cs index d2e4fa7301d75..2d6c85fa1c876 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.cs @@ -89,11 +89,6 @@ internal IpcServerChannel _ServerChannel get { return _serverChannel; } } - internal void Dispose(bool joinThreads) - { - Dispose(joinThreads, disposing: true); - } - #endregion private static string GenerateUniqueChannelLocalName() @@ -226,25 +221,21 @@ private bool CheckAlive(Process process) ~InteractiveHost() { - Dispose(joinThreads: false, disposing: false); + DisposeRemoteService(disposing: false); } public void Dispose() { - Dispose(joinThreads: false, disposing: true); + DisposeChannel(); + DisposeRemoteService(disposing: true); + GC.SuppressFinalize(this); } - private void Dispose(bool joinThreads, bool disposing) + private void DisposeRemoteService(bool disposing) { - if (disposing) - { - GC.SuppressFinalize(this); - DisposeChannel(); - } - if (_lazyRemoteService != null) { - _lazyRemoteService.Dispose(joinThreads); + _lazyRemoteService.Dispose(disposing); _lazyRemoteService = null; } } diff --git a/src/Interactive/HostTest/AbstractInteractiveHostTests.cs b/src/Interactive/HostTest/AbstractInteractiveHostTests.cs index 01f1c19c68b63..a49364013d6d2 100644 --- a/src/Interactive/HostTest/AbstractInteractiveHostTests.cs +++ b/src/Interactive/HostTest/AbstractInteractiveHostTests.cs @@ -29,7 +29,7 @@ internal static string GetInteractiveHostPath() internal static void DisposeInteractiveHostProcess(InteractiveHost process) { IpcServerChannel serverChannel = process._ServerChannel; - process.Dispose(joinThreads: true); + process.Dispose(); var listenerThread = (Thread)s_ipcServerChannelListenerThread.GetValue(serverChannel); listenerThread.Join();