Why the local method now vs. just a regular good old fashioned method? I realize it was nice when it was localized to this, but that ship has sailed. #Resolved

if [](start = 16, length = 2)

This doesn't guarantee that ProcessExitedHandler does not call _host.OnProcesExited after we return from Dispose.

There is a race condition:

1. ProcessExitedHandler has already been called and in it _disposing already tested, yet OnProcessExited has not been called yet
2. Dispose is called
3. Dispose returns
4. ProcessExitedHandler is resumed and calls OnProcessExited #Resolved

What's the benefit of using DisposableWait, vs. just _disposeLock.Wait(); _disposeLock.Release();? #Resolved

Also, we should probably not wait when running in finalizer (disposing == false).

In reply to: 160765822 [](ancestors = 160765822)

One more thing: Since we access _disposeLock in destructor we should guard against it being null, in case the constructor of this class doesn't run to completion.

var semaphore = _disposeSemaphore;
if (semaphore != null && disposing)
{
  semaphore.Wait();
  semaphore.Dispose();
}

In reply to: 160766053 [](ancestors = 160766053,160765822)

bool joinThreads, bool disposing [](start = 29, length = 32)

I think we should remove joinThreads and use disposing instead on line 250. We don't want to be waiting during finalization (on threads to join). #Resolved

GC.SuppressFinalize(this); [](start = 20, length = 26)

Let's move GC.SuppressFinalize(this) to the end of Dispose() to follow the common pattern. #Resolved

What pattern do you mean? I see various position of GC.SuppressFinalize(this) within Dispose(): in the beginning, in the middle and in the end.
What is the purpose to move it to the end?

In reply to: 160767337 [](ancestors = 160767337)

If the object is taken out of the finalizer queue and then dispose fails, it's never gonna be finalized. So the idea is that SuppressFinalize should be the last call of Dispose in order to avoid that. #Resolved

https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/dispose-pattern
#Resolved

_disposeLock [](start = 39, length = 12)

Let's call this _disposeSemaphore, since it's not actually a lock. #Resolved

using (await _disposeSemaphore.DisposableWaitAsync().ConfigureAwait(false)) [](start = 28, length = 75)

I'm not sure ho is this supposed to work. Should we just signal (call _disposeSemaphore.Release()) when this event is finished? #Resolved

For maintain some consistency I'd also signal the semaphore here (call _disposeSemaphore.Release()) and add a comment on that semaphore that it is signaled when the ProcessExited handler is removed and can no longer be executed. #Resolved

1 [](start = 91, length = 1)

Shouldn't this start in non-signaled state (initialCount: 0)?
Actually, since we only need two states we should use ManualResetEventSlim instead of SemaphoreSlim. #Resolved

One more thing: i think this should be moved to the RemoteService instance and used in its Dispose method. The InteractiveHost itself doesn't really need to know about this -- it's all about disposing the RemoteService.

In reply to: 162140846 [](ancestors = 162140846)

Let's move this to the RemoteService.Dispose() method.
#Resolved

_host != null [](start = 43, length = 13)

Good catch here, but there is still race. _host?.OnProcessExited(...) would be better. #Resolved

Actually, since _disposing must be true before _host is set to null, _host can't be null at this point.

Make _disposing volatile field though, so that the JIT doesn't reorder.

In reply to: 162224558 [](ancestors = 162224558)

object _disposeLock = new object(); #Resolved

if (Interlocked.Exchange(ref _processExitHandling, ProcessExitHandled) == ProcessExitHooked) [](start = 20, length = 92)

bool invokeEvent = false;

lock (_disposeLock) 
{
  if (_processExitHandling == ProcessExitHooked) 
  {
    Process.Exited -= ProcessExitedHandler;
     invokeEvent  =!_disposing;
    _processExitHandling = ProcessExitHandled;
  }
}

if (invokeEvent)
{
await _host.OnProcessExited(Process).ConfigureAwait(false);
}
``` #Resolved

lock (_disposeLock)
{
if (_processExitHandling == 0)
{
_processExitHandling = ProcessExitHooked;
Process.Exited += ProcessExitedHandler;
}
}
#Resolved

_disposing = true; [](start = 16, length = 18)

This should also be inside the using. #Resolved

Actually we don't need _disposing variable anymore.

In reply to: 162233741 [](ancestors = 162233741)

Agree. For consistency, it is better to put it inside using.

In reply to: 162233741 [](ancestors = 162233741)

Not just for consistency. Since this is now shared variable it would need to be under the lock. But we don't need it at all actually.

In reply to: 162234150 [](ancestors = 162234150,162233741)

if (!_disposing) [](start = 28, length = 16)

I think this check is not needed now. Once Dispose sets _processExitHandling to ProcessExitHandled we won't get here. #Resolved

int [](start = 29, length = 3)

Sorry, one more thing: now that we don't use Interlocked we could define the ProcessExitXXX values as an enum (ProcessExitHandlerStatus { Uninitialized = 0, Initialized = 1, Executed = 2 } ). Would make the code more readable.
#Resolved

Good idea! Thank you!

In reply to: 163422405 [](ancestors = 163422405)

_processExitHandling [](start = 42, length = 20)

nit: wrong name #Resolved

Why not just call the public overload? #Resolved

Is the problem here that this can deadlock because if the _host.OnProcessExited() call is happening above and that's still holding the lock, and it tries to call back into Dispose(), that will deadlock? Why can't we call _host.OnProcessExited() outside the lock?

I guess a better comment here would be appreciated. The use of volatile here is generally scary. I think it works, but that doesn't make it less scary. #Resolved

(this question is best answered by just adding more comments so the code is clearer vs. answering my question in GitHub) #Resolved

Thank you, Jason! I added more comments to clarify the scenario.

Maybe there can be an option to avoid using volatile and removing the _host.OnProcessExited() outside of the lock. We could not see these options so far. The problem we fixed that the host is being disposed during _host.OnProcessExited() execution. So, the fix is to protect it from the dispose.

In reply to: 163994639 [](ancestors = 163994639)