unregister onExit event handler on disposing interactive host

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
+            }
         }
     }
 }

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;
             }
         }

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();