From 4d9717f7f45da49982cda2c8aebbc2a3cbc61a87 Mon Sep 17 00:00:00 2001 From: Ivan Basov Date: Thu, 4 Jan 2018 16:15:42 -0800 Subject: [PATCH 01/19] unregister onExit event handler on disposing interactive host --- .../Interactive/Core/InteractiveHost.RemoteService.cs | 8 ++++++++ .../Features/Interactive/Core/InteractiveHost.cs | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs index ef8d0ff21a9f1..90c0e4697ccfd 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs @@ -16,6 +16,7 @@ internal sealed class RemoteService public readonly Process Process; public readonly Service Service; private readonly int _processId; + private EventHandler _currentLocalHandler; // output pumping threads (stream output from stdout/stderr of the host process to the output/errorOutput writers) private Thread _readOutputThread; // nulled on dispose @@ -61,6 +62,7 @@ async void localHandler(object _, EventArgs __) if (Interlocked.Exchange(ref processExitHandling, ProcessExitHandled) == ProcessExitHooked) { Process.Exited -= localHandler; + _currentLocalHandler = null; if (!_disposing) { @@ -78,6 +80,7 @@ async void localHandler(object _, EventArgs __) if (Interlocked.Exchange(ref processExitHandling, ProcessExitHooked) == 0) { Process.Exited += localHandler; + _currentLocalHandler = localHandler; } } @@ -115,6 +118,11 @@ internal void Dispose(bool joinThreads) { // set _disposing so that we don't attempt restart the host anymore: _disposing = true; + if (_currentLocalHandler != null) + { + Process.Exited -= _currentLocalHandler; + _currentLocalHandler = null; + } InitiateTermination(Process, _processId); diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.cs index d2e4fa7301d75..cb7d4534733c8 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.cs @@ -231,7 +231,7 @@ private bool CheckAlive(Process process) public void Dispose() { - Dispose(joinThreads: false, disposing: true); + Dispose(joinThreads: true, disposing: true); } private void Dispose(bool joinThreads, bool disposing) From d67d27288e1b6413ce2650a80099b0f30b901b27 Mon Sep 17 00:00:00 2001 From: Ivan Basov Date: Mon, 8 Jan 2018 14:15:46 -0800 Subject: [PATCH 02/19] code review feedback --- .../Core/InteractiveHost.RemoteService.cs | 45 +++++++++---------- 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs index 90c0e4697ccfd..7593c78cb5981 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs @@ -13,16 +13,19 @@ internal partial class InteractiveHost { internal sealed class RemoteService { + private const int ProcessExitHooked = 1; + private const int ProcessExitHandled = 2; + public readonly Process Process; public readonly Service Service; private readonly int _processId; - private EventHandler _currentLocalHandler; // 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 int _processExitHandling; // set to ProcessExitHandled on dispose internal RemoteService(InteractiveHost host, Process process, int processId, Service service) { @@ -35,6 +38,7 @@ internal RemoteService(InteractiveHost host, Process process, int processId, Ser this.Process = process; _processId = processId; this.Service = service; + _processExitHandling = 0; // TODO (tomat): consider using single-thread async readers _readOutputThread = new Thread(() => ReadOutput(error: false)); @@ -50,37 +54,30 @@ internal RemoteService(InteractiveHost host, Process process, int processId, Ser internal void HookAutoRestartEvent() { - const int ProcessExitHooked = 1; - const int ProcessExitHandled = 2; - - int processExitHandling = 0; + // hook the event only once per process: + if (Interlocked.Exchange(ref _processExitHandling, ProcessExitHooked) == 0) + { + Process.Exited += ProcessExitedHandler; + } + } - async void localHandler(object _, EventArgs __) + private async void ProcessExitedHandler(object _, EventArgs __) + { + try { - try + if (Interlocked.Exchange(ref _processExitHandling, ProcessExitHandled) == ProcessExitHooked) { - if (Interlocked.Exchange(ref processExitHandling, ProcessExitHandled) == ProcessExitHooked) - { - Process.Exited -= localHandler; - _currentLocalHandler = null; + Process.Exited -= ProcessExitedHandler; - if (!_disposing) - { - await _host.OnProcessExited(Process).ConfigureAwait(false); - } + if (!_disposing) + { + 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; - _currentLocalHandler = localHandler; + throw ExceptionUtilities.Unreachable; } } From 0262e8be31ef4c8317ebc4c5f6da36984038cba2 Mon Sep 17 00:00:00 2001 From: Ivan Basov Date: Mon, 8 Jan 2018 14:27:59 -0800 Subject: [PATCH 03/19] fixing incomplete commit --- .../Interactive/Core/InteractiveHost.RemoteService.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs index 7593c78cb5981..ce19ffe309571 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs @@ -115,10 +115,9 @@ internal void Dispose(bool joinThreads) { // set _disposing so that we don't attempt restart the host anymore: _disposing = true; - if (_currentLocalHandler != null) + if (Interlocked.Exchange(ref _processExitHandling, ProcessExitHandled) == ProcessExitHooked) { - Process.Exited -= _currentLocalHandler; - _currentLocalHandler = null; + Process.Exited -= ProcessExitedHandler; } InitiateTermination(Process, _processId); From cb57d2dfaf2c52e73be9a5b8a01a4fa768a0101b Mon Sep 17 00:00:00 2001 From: Ivan Basov Date: Mon, 8 Jan 2018 18:47:56 -0800 Subject: [PATCH 04/19] code review feedback --- .../Core/InteractiveHost.RemoteService.cs | 13 +++++++++-- .../Interactive/Core/InteractiveHost.cs | 22 +++++++++++-------- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs index ce19ffe309571..c836a557089e1 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs @@ -19,6 +19,7 @@ internal sealed class RemoteService public readonly Process Process; public readonly Service Service; private readonly int _processId; + private readonly SemaphoreSlim _disposeLock; // output pumping threads (stream output from stdout/stderr of the host process to the output/errorOutput writers) private Thread _readOutputThread; // nulled on dispose @@ -27,11 +28,12 @@ internal sealed class RemoteService private bool _disposing; // set to true on dispose private int _processExitHandling; // set to ProcessExitHandled on dispose - internal RemoteService(InteractiveHost host, Process process, int processId, Service service) + internal RemoteService(InteractiveHost host, Process process, int processId, Service service, SemaphoreSlim disposeLock) { Debug.Assert(host != null); Debug.Assert(process != null); Debug.Assert(service != null); + Debug.Assert(disposeLock != null); _host = host; _disposing = false; @@ -39,6 +41,7 @@ internal RemoteService(InteractiveHost host, Process process, int processId, Ser _processId = processId; this.Service = service; _processExitHandling = 0; + _disposeLock = disposeLock; // TODO (tomat): consider using single-thread async readers _readOutputThread = new Thread(() => ReadOutput(error: false)); @@ -71,7 +74,13 @@ private async void ProcessExitedHandler(object _, EventArgs __) if (!_disposing) { - await _host.OnProcessExited(Process).ConfigureAwait(false); + using (await _disposeLock.DisposableWaitAsync().ConfigureAwait(false)) + { + if (_host != null) + { + await _host.OnProcessExited(Process).ConfigureAwait(false); + } + } } } } diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.cs index cb7d4534733c8..6623a581522d6 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.cs @@ -31,6 +31,7 @@ internal sealed partial class InteractiveHost : MarshalByRefObject // adjustable for testing purposes private readonly int _millisecondsTimeout; private const int MaxAttemptsToCreateProcess = 2; + private readonly SemaphoreSlim _disposeLock = new SemaphoreSlim(initialCount: 1); private LazyRemoteService _lazyRemoteService; private int _remoteServiceInstanceId; @@ -193,7 +194,7 @@ private RemoteService TryStartProcess(CultureInfo culture, CancellationToken can return null; } - return new RemoteService(this, newProcess, newProcessId, newService); + return new RemoteService(this, newProcess, newProcessId, newService, _disposeLock); } catch (OperationCanceledException) { @@ -236,16 +237,19 @@ public void Dispose() private void Dispose(bool joinThreads, bool disposing) { - if (disposing) + using (_disposeLock.DisposableWait()) { - GC.SuppressFinalize(this); - DisposeChannel(); - } + if (disposing) + { + GC.SuppressFinalize(this); + DisposeChannel(); + } - if (_lazyRemoteService != null) - { - _lazyRemoteService.Dispose(joinThreads); - _lazyRemoteService = null; + if (_lazyRemoteService != null) + { + _lazyRemoteService.Dispose(joinThreads); + _lazyRemoteService = null; + } } } From a3fce2b00120692e74d3cbec88e65836add6842c Mon Sep 17 00:00:00 2001 From: Ivan Basov Date: Wed, 10 Jan 2018 15:57:48 -0800 Subject: [PATCH 05/19] code review feedback --- .../Core/InteractiveHost.RemoteService.cs | 10 ++--- .../Interactive/Core/InteractiveHost.cs | 45 ++++++++++--------- .../HostTest/AbstractInteractiveHostTests.cs | 2 +- 3 files changed, 30 insertions(+), 27 deletions(-) diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs index c836a557089e1..869a1282624c4 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs @@ -19,7 +19,7 @@ internal sealed class RemoteService public readonly Process Process; public readonly Service Service; private readonly int _processId; - private readonly SemaphoreSlim _disposeLock; + private readonly SemaphoreSlim _disposeSemaphore; // output pumping threads (stream output from stdout/stderr of the host process to the output/errorOutput writers) private Thread _readOutputThread; // nulled on dispose @@ -28,12 +28,12 @@ internal sealed class RemoteService private bool _disposing; // set to true on dispose private int _processExitHandling; // set to ProcessExitHandled on dispose - internal RemoteService(InteractiveHost host, Process process, int processId, Service service, SemaphoreSlim disposeLock) + internal RemoteService(InteractiveHost host, Process process, int processId, Service service, SemaphoreSlim disposeSemaphore) { Debug.Assert(host != null); Debug.Assert(process != null); Debug.Assert(service != null); - Debug.Assert(disposeLock != null); + Debug.Assert(disposeSemaphore != null); _host = host; _disposing = false; @@ -41,7 +41,7 @@ internal RemoteService(InteractiveHost host, Process process, int processId, Ser _processId = processId; this.Service = service; _processExitHandling = 0; - _disposeLock = disposeLock; + _disposeSemaphore = disposeSemaphore; // TODO (tomat): consider using single-thread async readers _readOutputThread = new Thread(() => ReadOutput(error: false)); @@ -74,7 +74,7 @@ private async void ProcessExitedHandler(object _, EventArgs __) if (!_disposing) { - using (await _disposeLock.DisposableWaitAsync().ConfigureAwait(false)) + using (await _disposeSemaphore.DisposableWaitAsync().ConfigureAwait(false)) { if (_host != null) { diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.cs index 6623a581522d6..0825034f0b4ea 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.cs @@ -31,7 +31,7 @@ internal sealed partial class InteractiveHost : MarshalByRefObject // adjustable for testing purposes private readonly int _millisecondsTimeout; private const int MaxAttemptsToCreateProcess = 2; - private readonly SemaphoreSlim _disposeLock = new SemaphoreSlim(initialCount: 1); + private readonly SemaphoreSlim _disposeSemaphore = new SemaphoreSlim(initialCount: 1); private LazyRemoteService _lazyRemoteService; private int _remoteServiceInstanceId; @@ -90,11 +90,6 @@ internal IpcServerChannel _ServerChannel get { return _serverChannel; } } - internal void Dispose(bool joinThreads) - { - Dispose(joinThreads, disposing: true); - } - #endregion private static string GenerateUniqueChannelLocalName() @@ -194,7 +189,7 @@ private RemoteService TryStartProcess(CultureInfo culture, CancellationToken can return null; } - return new RemoteService(this, newProcess, newProcessId, newService, _disposeLock); + return new RemoteService(this, newProcess, newProcessId, newService, _disposeSemaphore); } catch (OperationCanceledException) { @@ -227,29 +222,37 @@ private bool CheckAlive(Process process) ~InteractiveHost() { - Dispose(joinThreads: false, disposing: false); + Dispose(disposing: false); } public void Dispose() { - Dispose(joinThreads: true, disposing: true); + Dispose(disposing: true); } - private void Dispose(bool joinThreads, bool disposing) + internal void Dispose(bool disposing) { - using (_disposeLock.DisposableWait()) + var semaphore = _disposeSemaphore; + if (semaphore != null && disposing) { - if (disposing) - { - GC.SuppressFinalize(this); - DisposeChannel(); - } + semaphore.Wait(); + semaphore.Dispose(); + } - if (_lazyRemoteService != null) - { - _lazyRemoteService.Dispose(joinThreads); - _lazyRemoteService = null; - } + if (disposing) + { + DisposeChannel(); + } + + if (_lazyRemoteService != null) + { + _lazyRemoteService.Dispose(disposing); + _lazyRemoteService = null; + } + + if (disposing) + { + GC.SuppressFinalize(this); } } diff --git a/src/Interactive/HostTest/AbstractInteractiveHostTests.cs b/src/Interactive/HostTest/AbstractInteractiveHostTests.cs index 01f1c19c68b63..2ed3788347fc2 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(disposing: true); var listenerThread = (Thread)s_ipcServerChannelListenerThread.GetValue(serverChannel); listenerThread.Join(); From e625b6dda1fa906cff08e074da4639dad95acf68 Mon Sep 17 00:00:00 2001 From: Ivan Basov Date: Wed, 17 Jan 2018 16:48:53 -0800 Subject: [PATCH 06/19] code review feedback --- .../Core/InteractiveHost.RemoteService.cs | 72 +++++++++++-------- .../Interactive/Core/InteractiveHost.cs | 10 +-- 2 files changed, 44 insertions(+), 38 deletions(-) diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs index 869a1282624c4..98bf7fd27aee0 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; @@ -19,7 +20,8 @@ internal sealed class RemoteService public readonly Process Process; public readonly Service Service; private readonly int _processId; - private readonly SemaphoreSlim _disposeSemaphore; + // The semaphore is reset (closed) by default: + private readonly ManualResetEventSlim _disposeSemaphore = new ManualResetEventSlim(initialState: false); // output pumping threads (stream output from stdout/stderr of the host process to the output/errorOutput writers) private Thread _readOutputThread; // nulled on dispose @@ -28,12 +30,11 @@ internal sealed class RemoteService private bool _disposing; // set to true on dispose private int _processExitHandling; // set to ProcessExitHandled on dispose - internal RemoteService(InteractiveHost host, Process process, int processId, Service service, SemaphoreSlim disposeSemaphore) + internal RemoteService(InteractiveHost host, Process process, int processId, Service service) { Debug.Assert(host != null); Debug.Assert(process != null); Debug.Assert(service != null); - Debug.Assert(disposeSemaphore != null); _host = host; _disposing = false; @@ -41,7 +42,6 @@ internal RemoteService(InteractiveHost host, Process process, int processId, Ser _processId = processId; this.Service = service; _processExitHandling = 0; - _disposeSemaphore = disposeSemaphore; // TODO (tomat): consider using single-thread async readers _readOutputThread = new Thread(() => ReadOutput(error: false)); @@ -72,16 +72,12 @@ private async void ProcessExitedHandler(object _, EventArgs __) { Process.Exited -= ProcessExitedHandler; - if (!_disposing) + if (!_disposing && _host != null) { - using (await _disposeSemaphore.DisposableWaitAsync().ConfigureAwait(false)) - { - if (_host != null) - { - await _host.OnProcessExited(Process).ConfigureAwait(false); - } - } + await _host.OnProcessExited(Process).ConfigureAwait(false); } + + _disposeSemaphore.Set(); } } catch (Exception e) when (FatalError.Report(e)) @@ -124,39 +120,57 @@ internal void Dispose(bool joinThreads) { // set _disposing so that we don't attempt restart the host anymore: _disposing = true; + if (Interlocked.Exchange(ref _processExitHandling, ProcessExitHandled) == ProcessExitHooked) { Process.Exited -= ProcessExitedHandler; + _disposeSemaphore.Set(); + } + + using (var semaphore = _disposeSemaphore) + { + if (semaphore != null) + { + // This semaphore should be set either in the block just above or by completion of another thread that was able to execute Interlocked.Exchange before. + semaphore.Wait(); + } } InitiateTermination(Process, _processId); - // only tests require joining the threads, so we can wait synchronously if (joinThreads) { - if (_readOutputThread != null) + var readOutputThreadJoinTask = Task.Run(() => { - try + if (_readOutputThread != null) { - _readOutputThread.Join(); - } - catch (ThreadStateException) - { - // thread hasn't started + try + { + _readOutputThread.Join(); + } + catch (ThreadStateException) + { + // thread hasn't started + } } - } + }); - if (_readErrorOutputThread != null) + var readErrorOutputThreadJoinTask = Task.Run(() => { - try - { - _readErrorOutputThread.Join(); - } - catch (ThreadStateException) + if (_readErrorOutputThread != null) { - // thread hasn't started + try + { + _readErrorOutputThread.Join(); + } + catch (ThreadStateException) + { + // thread hasn't started + } } - } + }); + + Task.WaitAll(readOutputThreadJoinTask, readErrorOutputThreadJoinTask); } // null the host so that we don't attempt to write to the buffer anymore: diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.cs index 0825034f0b4ea..8990db173693f 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.cs @@ -31,7 +31,6 @@ internal sealed partial class InteractiveHost : MarshalByRefObject // adjustable for testing purposes private readonly int _millisecondsTimeout; private const int MaxAttemptsToCreateProcess = 2; - private readonly SemaphoreSlim _disposeSemaphore = new SemaphoreSlim(initialCount: 1); private LazyRemoteService _lazyRemoteService; private int _remoteServiceInstanceId; @@ -189,7 +188,7 @@ private RemoteService TryStartProcess(CultureInfo culture, CancellationToken can return null; } - return new RemoteService(this, newProcess, newProcessId, newService, _disposeSemaphore); + return new RemoteService(this, newProcess, newProcessId, newService); } catch (OperationCanceledException) { @@ -232,13 +231,6 @@ public void Dispose() internal void Dispose(bool disposing) { - var semaphore = _disposeSemaphore; - if (semaphore != null && disposing) - { - semaphore.Wait(); - semaphore.Dispose(); - } - if (disposing) { DisposeChannel(); From 91a88102d3a9e43151ad9e10ddf781067dc4cd3f Mon Sep 17 00:00:00 2001 From: Ivan Basov Date: Wed, 17 Jan 2018 18:09:14 -0800 Subject: [PATCH 07/19] code review feedback --- .../Core/InteractiveHost.RemoteService.cs | 43 +++++++++---------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs index 98bf7fd27aee0..8eb1d9a7228b0 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs @@ -20,8 +20,7 @@ internal sealed class RemoteService public readonly Process Process; public readonly Service Service; private readonly int _processId; - // The semaphore is reset (closed) by default: - private readonly ManualResetEventSlim _disposeSemaphore = new ManualResetEventSlim(initialState: false); + private SemaphoreSlim _disposeSemaphore = new SemaphoreSlim(initialCount: 0); // output pumping threads (stream output from stdout/stderr of the host process to the output/errorOutput writers) private Thread _readOutputThread; // nulled on dispose @@ -57,10 +56,14 @@ internal RemoteService(InteractiveHost host, Process process, int processId, Ser internal void HookAutoRestartEvent() { - // hook the event only once per process: - if (Interlocked.Exchange(ref _processExitHandling, ProcessExitHooked) == 0) + using(_disposeSemaphore.DisposableWait()) { - Process.Exited += ProcessExitedHandler; + // hook the event only once per process: + if (_processExitHandling == 0) + { + Process.Exited += ProcessExitedHandler; + _processExitHandling = ProcessExitHooked; + } } } @@ -68,16 +71,18 @@ private async void ProcessExitedHandler(object _, EventArgs __) { try { - if (Interlocked.Exchange(ref _processExitHandling, ProcessExitHandled) == ProcessExitHooked) + using (await _disposeSemaphore.DisposableWaitAsync().ConfigureAwait(false)) { - Process.Exited -= ProcessExitedHandler; - - if (!_disposing && _host != null) + if (_processExitHandling == ProcessExitHooked) { - await _host.OnProcessExited(Process).ConfigureAwait(false); - } + Process.Exited -= ProcessExitedHandler; + if (!_disposing) + { + await _host.OnProcessExited(Process).ConfigureAwait(false); + } - _disposeSemaphore.Set(); + _processExitHandling = ProcessExitHandled; + } } } catch (Exception e) when (FatalError.Report(e)) @@ -121,18 +126,12 @@ internal void Dispose(bool joinThreads) // set _disposing so that we don't attempt restart the host anymore: _disposing = true; - if (Interlocked.Exchange(ref _processExitHandling, ProcessExitHandled) == ProcessExitHooked) - { - Process.Exited -= ProcessExitedHandler; - _disposeSemaphore.Set(); - } - - using (var semaphore = _disposeSemaphore) + using(_disposeSemaphore.DisposableWait()) { - if (semaphore != null) + if (_processExitHandling == ProcessExitHooked) { - // This semaphore should be set either in the block just above or by completion of another thread that was able to execute Interlocked.Exchange before. - semaphore.Wait(); + Process.Exited -= ProcessExitedHandler; + _processExitHandling = ProcessExitHandled; } } From bb185c72546e3e3e63e94291a5c7dcc5fe9024a0 Mon Sep 17 00:00:00 2001 From: Ivan Basov Date: Wed, 17 Jan 2018 18:30:46 -0800 Subject: [PATCH 08/19] code review feedback --- .../Interactive/Core/InteractiveHost.RemoteService.cs | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs index 8eb1d9a7228b0..7c1434560029e 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs @@ -26,7 +26,6 @@ internal sealed class RemoteService 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 int _processExitHandling; // set to ProcessExitHandled on dispose internal RemoteService(InteractiveHost host, Process process, int processId, Service service) @@ -36,7 +35,6 @@ 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; @@ -76,11 +74,7 @@ private async void ProcessExitedHandler(object _, EventArgs __) if (_processExitHandling == ProcessExitHooked) { Process.Exited -= ProcessExitedHandler; - if (!_disposing) - { - await _host.OnProcessExited(Process).ConfigureAwait(false); - } - + await _host.OnProcessExited(Process).ConfigureAwait(false); _processExitHandling = ProcessExitHandled; } } @@ -123,9 +117,6 @@ private void ReadOutput(bool error) internal void Dispose(bool joinThreads) { - // set _disposing so that we don't attempt restart the host anymore: - _disposing = true; - using(_disposeSemaphore.DisposableWait()) { if (_processExitHandling == ProcessExitHooked) From 740a6961782c167335ecc3232487506c73c1bbc8 Mon Sep 17 00:00:00 2001 From: Ivan Basov Date: Thu, 18 Jan 2018 16:07:19 -0800 Subject: [PATCH 09/19] rebase --- .../Core/InteractiveHost.RemoteService.cs | 30 ++++++++----------- 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs index 7c1434560029e..c0a00f1780b13 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs @@ -132,31 +132,25 @@ internal void Dispose(bool joinThreads) { var readOutputThreadJoinTask = Task.Run(() => { - if (_readOutputThread != null) + try { - try - { - _readOutputThread.Join(); - } - catch (ThreadStateException) - { - // thread hasn't started - } + _readOutputThread?.Join(); + } + catch (ThreadStateException) + { + // thread hasn't started } }); var readErrorOutputThreadJoinTask = Task.Run(() => { - if (_readErrorOutputThread != null) + try + { + _readErrorOutputThread?.Join(); + } + catch (ThreadStateException) { - try - { - _readErrorOutputThread.Join(); - } - catch (ThreadStateException) - { - // thread hasn't started - } + // thread hasn't started } }); From 2977818a5dbc3a60e3bd60760859b770b7c75288 Mon Sep 17 00:00:00 2001 From: Ivan Basov Date: Thu, 18 Jan 2018 18:09:42 -0800 Subject: [PATCH 10/19] experiment #1 --- .../Features/Interactive/Core/InteractiveHost.RemoteService.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs index c0a00f1780b13..58b96e34a6c88 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs @@ -74,7 +74,7 @@ private async void ProcessExitedHandler(object _, EventArgs __) if (_processExitHandling == ProcessExitHooked) { Process.Exited -= ProcessExitedHandler; - await _host.OnProcessExited(Process).ConfigureAwait(false); + // await _host.OnProcessExited(Process).ConfigureAwait(false); _processExitHandling = ProcessExitHandled; } } From eb87327661ad756ea16d92f44fe33a2c29d77983 Mon Sep 17 00:00:00 2001 From: Ivan Basov Date: Fri, 19 Jan 2018 12:10:09 -0800 Subject: [PATCH 11/19] experiment #2 --- .../Core/InteractiveHost.RemoteService.cs | 38 ++++++++----------- 1 file changed, 15 insertions(+), 23 deletions(-) diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs index 58b96e34a6c88..9c44b55761311 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs @@ -74,7 +74,7 @@ private async void ProcessExitedHandler(object _, EventArgs __) if (_processExitHandling == ProcessExitHooked) { Process.Exited -= ProcessExitedHandler; - // await _host.OnProcessExited(Process).ConfigureAwait(false); + await _host.OnProcessExited(Process).ConfigureAwait(false); _processExitHandling = ProcessExitHandled; } } @@ -130,31 +130,23 @@ internal void Dispose(bool joinThreads) if (joinThreads) { - var readOutputThreadJoinTask = Task.Run(() => + try { - try - { - _readOutputThread?.Join(); - } - catch (ThreadStateException) - { - // thread hasn't started - } - }); - - var readErrorOutputThreadJoinTask = Task.Run(() => + _readOutputThread?.Join(); + } + catch (ThreadStateException) { - try - { - _readErrorOutputThread?.Join(); - } - catch (ThreadStateException) - { - // thread hasn't started - } - }); + // thread hasn't started + } - Task.WaitAll(readOutputThreadJoinTask, readErrorOutputThreadJoinTask); + try + { + _readErrorOutputThread?.Join(); + } + catch (ThreadStateException) + { + // thread hasn't started + } } // null the host so that we don't attempt to write to the buffer anymore: From 30bd67ee81c287e936b3229459460aa8f9694dea Mon Sep 17 00:00:00 2001 From: Ivan Basov Date: Fri, 19 Jan 2018 14:18:22 -0800 Subject: [PATCH 12/19] experiment #3 --- .../Core/InteractiveHost.RemoteService.cs | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs index 9c44b55761311..122473307167f 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs @@ -128,26 +128,26 @@ internal void Dispose(bool joinThreads) InitiateTermination(Process, _processId); - if (joinThreads) - { - try - { - _readOutputThread?.Join(); - } - catch (ThreadStateException) - { - // thread hasn't started - } - - try - { - _readErrorOutputThread?.Join(); - } - catch (ThreadStateException) - { - // thread hasn't started - } - } + //if (joinThreads) + //{ + // 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: _host = null; From e299bf8be0efee1f22f10f486a2b0f192ddb2528 Mon Sep 17 00:00:00 2001 From: Ivan Basov Date: Fri, 19 Jan 2018 16:14:37 -0800 Subject: [PATCH 13/19] Experiment #4 --- .../Features/Interactive/Core/InteractiveHost.RemoteService.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs index 122473307167f..13e73f5ecb537 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs @@ -26,7 +26,7 @@ internal sealed class RemoteService private Thread _readOutputThread; // nulled on dispose private Thread _readErrorOutputThread; // nulled on dispose private InteractiveHost _host; // nulled on dispose - private int _processExitHandling; // set to ProcessExitHandled on dispose + private volatile int _processExitHandling; // set to ProcessExitHandled on dispose internal RemoteService(InteractiveHost host, Process process, int processId, Service service) { From 9abe6076db44a44a8bca3e4ff0934037fa689f79 Mon Sep 17 00:00:00 2001 From: Ivan Basov Date: Fri, 19 Jan 2018 18:19:00 -0800 Subject: [PATCH 14/19] semaphore init corrected --- .../Core/InteractiveHost.RemoteService.cs | 49 ++++++++++--------- 1 file changed, 27 insertions(+), 22 deletions(-) diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs index 13e73f5ecb537..dd72a88560986 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs @@ -20,13 +20,13 @@ internal sealed class RemoteService public readonly Process Process; public readonly Service Service; private readonly int _processId; - private SemaphoreSlim _disposeSemaphore = new SemaphoreSlim(initialCount: 0); + 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 volatile int _processExitHandling; // set to ProcessExitHandled on dispose + private int _processExitHandling; // set to ProcessExitHandled on dispose internal RemoteService(InteractiveHost host, Process process, int processId, Service service) { @@ -128,26 +128,31 @@ internal void Dispose(bool joinThreads) InitiateTermination(Process, _processId); - //if (joinThreads) - //{ - // try - // { - // _readOutputThread?.Join(); - // } - // catch (ThreadStateException) - // { - // // thread hasn't started - // } - - // try - // { - // _readErrorOutputThread?.Join(); - // } - // catch (ThreadStateException) - // { - // // thread hasn't started - // } - //} + var readOutputThreadJoinTask = Task.Run(() => + { + try + { + _readOutputThread?.Join(); + } + catch (ThreadStateException) + { + // thread hasn't started + } + }); + + var readErrorOutputThreadJoinTask = Task.Run(() => + { + try + { + _readErrorOutputThread?.Join(); + } + catch (ThreadStateException) + { + // thread hasn't started + } + }); + + Task.WaitAll(readOutputThreadJoinTask, readErrorOutputThreadJoinTask); // null the host so that we don't attempt to write to the buffer anymore: _host = null; From ac9e071bb3279240f173d8326452695005d37520 Mon Sep 17 00:00:00 2001 From: Ivan Basov Date: Tue, 23 Jan 2018 11:32:19 -0800 Subject: [PATCH 15/19] resolving a deadlock --- .../Core/InteractiveHost.RemoteService.cs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs index dd72a88560986..ba7e7f5d0416a 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs @@ -74,8 +74,9 @@ private async void ProcessExitedHandler(object _, EventArgs __) if (_processExitHandling == ProcessExitHooked) { Process.Exited -= ProcessExitedHandler; - await _host.OnProcessExited(Process).ConfigureAwait(false); _processExitHandling = ProcessExitHandled; + // Should set _processExitHandling before calling OnProcessExited to avoid deadlocks. + await _host.OnProcessExited(Process).ConfigureAwait(false); } } } @@ -117,12 +118,17 @@ private void ReadOutput(bool error) internal void Dispose(bool joinThreads) { - using(_disposeSemaphore.DisposableWait()) + // There can be a call from host initiated from OnProcessExit. + // This check on the beginning helps to avoid the circular reference. + if (_processExitHandling == ProcessExitHooked) { - if (_processExitHandling == ProcessExitHooked) + using (_disposeSemaphore.DisposableWait()) { - Process.Exited -= ProcessExitedHandler; - _processExitHandling = ProcessExitHandled; + if (_processExitHandling == ProcessExitHooked) + { + Process.Exited -= ProcessExitedHandler; + _processExitHandling = ProcessExitHandled; + } } } From 68dfd2df767bb5a1bc3d9f3c8ea4fa91b0aab2f5 Mon Sep 17 00:00:00 2001 From: Ivan Basov Date: Tue, 23 Jan 2018 14:12:25 -0800 Subject: [PATCH 16/19] feedback --- .../Core/InteractiveHost.RemoteService.cs | 40 ++++++++----------- 1 file changed, 16 insertions(+), 24 deletions(-) diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs index ba7e7f5d0416a..b80d74d4ed7f3 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs @@ -26,7 +26,7 @@ internal sealed class RemoteService private Thread _readOutputThread; // nulled on dispose private Thread _readErrorOutputThread; // nulled on dispose private InteractiveHost _host; // nulled on dispose - private int _processExitHandling; // set to ProcessExitHandled on dispose + private volatile int _processExitHandling; // set to ProcessExitHandled on dispose internal RemoteService(InteractiveHost host, Process process, int processId, Service service) { @@ -119,7 +119,7 @@ private void ReadOutput(bool error) internal void Dispose(bool joinThreads) { // There can be a call from host initiated from OnProcessExit. - // This check on the beginning helps to avoid the circular reference. + // This check on the beginning helps to avoid a reentrancy. if (_processExitHandling == ProcessExitHooked) { using (_disposeSemaphore.DisposableWait()) @@ -134,31 +134,23 @@ internal void Dispose(bool joinThreads) InitiateTermination(Process, _processId); - var readOutputThreadJoinTask = Task.Run(() => + try { - try - { - _readOutputThread?.Join(); - } - catch (ThreadStateException) - { - // thread hasn't started - } - }); - - var readErrorOutputThreadJoinTask = Task.Run(() => + _readOutputThread?.Join(); + } + catch (ThreadStateException) { - try - { - _readErrorOutputThread?.Join(); - } - catch (ThreadStateException) - { - // thread hasn't started - } - }); + // thread hasn't started + } - Task.WaitAll(readOutputThreadJoinTask, readErrorOutputThreadJoinTask); + try + { + _readErrorOutputThread?.Join(); + } + catch (ThreadStateException) + { + // thread hasn't started + } // null the host so that we don't attempt to write to the buffer anymore: _host = null; From 49f7bf13fede53ea6ddd914788c966c796668bc4 Mon Sep 17 00:00:00 2001 From: Ivan Basov Date: Tue, 23 Jan 2018 16:30:22 -0800 Subject: [PATCH 17/19] feedback --- .../Core/InteractiveHost.RemoteService.cs | 30 +++++++++++-------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs index b80d74d4ed7f3..144344f2de95b 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs @@ -14,9 +14,6 @@ internal partial class InteractiveHost { internal sealed class RemoteService { - private const int ProcessExitHooked = 1; - private const int ProcessExitHandled = 2; - public readonly Process Process; public readonly Service Service; private readonly int _processId; @@ -26,7 +23,7 @@ internal sealed class RemoteService private Thread _readOutputThread; // nulled on dispose private Thread _readErrorOutputThread; // nulled on dispose private InteractiveHost _host; // nulled on dispose - private volatile int _processExitHandling; // set to ProcessExitHandled on dispose + private volatile ProcessExitHandlerStatus _processExitHandlerStatus; // set to Handled on dispose internal RemoteService(InteractiveHost host, Process process, int processId, Service service) { @@ -38,7 +35,7 @@ internal RemoteService(InteractiveHost host, Process process, int processId, Ser this.Process = process; _processId = processId; this.Service = service; - _processExitHandling = 0; + _processExitHandlerStatus = ProcessExitHandlerStatus.Uninitialized; // TODO (tomat): consider using single-thread async readers _readOutputThread = new Thread(() => ReadOutput(error: false)); @@ -54,13 +51,13 @@ internal RemoteService(InteractiveHost host, Process process, int processId, Ser internal void HookAutoRestartEvent() { - using(_disposeSemaphore.DisposableWait()) + using (_disposeSemaphore.DisposableWait()) { // hook the event only once per process: - if (_processExitHandling == 0) + if (_processExitHandlerStatus == ProcessExitHandlerStatus.Uninitialized) { Process.Exited += ProcessExitedHandler; - _processExitHandling = ProcessExitHooked; + _processExitHandlerStatus = ProcessExitHandlerStatus.Hooked; } } } @@ -71,10 +68,10 @@ private async void ProcessExitedHandler(object _, EventArgs __) { using (await _disposeSemaphore.DisposableWaitAsync().ConfigureAwait(false)) { - if (_processExitHandling == ProcessExitHooked) + if (_processExitHandlerStatus == ProcessExitHandlerStatus.Hooked) { Process.Exited -= ProcessExitedHandler; - _processExitHandling = ProcessExitHandled; + _processExitHandlerStatus = ProcessExitHandlerStatus.Handled; // Should set _processExitHandling before calling OnProcessExited to avoid deadlocks. await _host.OnProcessExited(Process).ConfigureAwait(false); } @@ -120,14 +117,14 @@ internal void Dispose(bool joinThreads) { // There can be a call from host initiated from OnProcessExit. // This check on the beginning helps to avoid a reentrancy. - if (_processExitHandling == ProcessExitHooked) + if (_processExitHandlerStatus == ProcessExitHandlerStatus.Hooked) { using (_disposeSemaphore.DisposableWait()) { - if (_processExitHandling == ProcessExitHooked) + if (_processExitHandlerStatus == ProcessExitHandlerStatus.Hooked) { Process.Exited -= ProcessExitedHandler; - _processExitHandling = ProcessExitHandled; + _processExitHandlerStatus = ProcessExitHandlerStatus.Handled; } } } @@ -172,6 +169,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 + } } } } From 2243d2a7b28f1f7de0e11fb9edbc4980c3ce7f29 Mon Sep 17 00:00:00 2001 From: Ivan Basov Date: Tue, 23 Jan 2018 16:57:30 -0800 Subject: [PATCH 18/19] comment correction --- .../Features/Interactive/Core/InteractiveHost.RemoteService.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs index 144344f2de95b..4afe6af6b7686 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs @@ -72,7 +72,7 @@ private async void ProcessExitedHandler(object _, EventArgs __) { Process.Exited -= ProcessExitedHandler; _processExitHandlerStatus = ProcessExitHandlerStatus.Handled; - // Should set _processExitHandling before calling OnProcessExited to avoid deadlocks. + // Should set _processExitHandlerStatus before calling OnProcessExited to avoid deadlocks. await _host.OnProcessExited(Process).ConfigureAwait(false); } } From 5650219af2c8fbadb9b828758a5ceacdcc162c0e Mon Sep 17 00:00:00 2001 From: Ivan Basov Date: Thu, 25 Jan 2018 16:05:19 -0800 Subject: [PATCH 19/19] code review feedback --- .../Core/InteractiveHost.RemoteService.cs | 1 + .../Interactive/Core/InteractiveHost.cs | 18 +++++------------- .../HostTest/AbstractInteractiveHostTests.cs | 2 +- 3 files changed, 7 insertions(+), 14 deletions(-) diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs index 4afe6af6b7686..e3e764942c863 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs @@ -73,6 +73,7 @@ private async void ProcessExitedHandler(object _, EventArgs __) 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); } } diff --git a/src/Interactive/Features/Interactive/Core/InteractiveHost.cs b/src/Interactive/Features/Interactive/Core/InteractiveHost.cs index 8990db173693f..2d6c85fa1c876 100644 --- a/src/Interactive/Features/Interactive/Core/InteractiveHost.cs +++ b/src/Interactive/Features/Interactive/Core/InteractiveHost.cs @@ -221,31 +221,23 @@ private bool CheckAlive(Process process) ~InteractiveHost() { - Dispose(disposing: false); + DisposeRemoteService(disposing: false); } public void Dispose() { - Dispose(disposing: true); + DisposeChannel(); + DisposeRemoteService(disposing: true); + GC.SuppressFinalize(this); } - internal void Dispose(bool disposing) + private void DisposeRemoteService(bool disposing) { - if (disposing) - { - DisposeChannel(); - } - if (_lazyRemoteService != null) { _lazyRemoteService.Dispose(disposing); _lazyRemoteService = null; } - - if (disposing) - { - GC.SuppressFinalize(this); - } } private void DisposeChannel() diff --git a/src/Interactive/HostTest/AbstractInteractiveHostTests.cs b/src/Interactive/HostTest/AbstractInteractiveHostTests.cs index 2ed3788347fc2..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(disposing: true); + process.Dispose(); var listenerThread = (Thread)s_ipcServerChannelListenerThread.GetValue(serverChannel); listenerThread.Join();