unregister onExit event handler on disposing interactive host #24058

ivanbasov · 2018-01-05T00:18:04Z

Customer scenario

Customer shuts down Visual Studio. There is a race condition when closing the Interactive Window and the corresponding process.

When the process is closed before the window (within this or another scenario), it sends a message to display it in the interactive window. If the window was closed somewhere between checks the it is open and executing a command to output the message, an failure happens.

Bugs this fixes

VSO 514822 and corresponding: #24006

Workarounds, if any

None

Risk

Low

Performance impact

None

Is this a regression from a previous update?

No

How was the bug found?

Watson hits

jasonmalinowski · 2018-01-05T23:41:00Z

src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs

@@ -61,6 +62,7 @@ internal void HookAutoRestartEvent()
                        if (Interlocked.Exchange(ref processExitHandling, ProcessExitHandled) == ProcessExitHooked)
                        {
                            Process.Exited -= localHandler;
+                            _currentLocalHandler = null;


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

jasonmalinowski

I think tossing the local method now makes this easier to follow.

tmat · 2018-01-08T23:09:52Z

src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs

@@ -115,6 +115,10 @@ 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)


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:

ProcessExitedHandler has already been called and in it _disposing already tested, yet OnProcessExited has not been called yet

Dispose is called

Dispose returns

ProcessExitedHandler is resumed and calls OnProcessExited #Resolved

tmat

🕐

tmat · 2018-01-10T18:49:09Z

src/Interactive/Features/Interactive/Core/InteractiveHost.cs

        }

        private void Dispose(bool joinThreads, bool disposing)
        {
-            if (disposing)
+            using (_disposeLock.DisposableWait())
            {


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)

tmat · 2018-01-10T18:51:31Z

src/Interactive/Features/Interactive/Core/InteractiveHost.cs

@@ -231,21 +232,24 @@ 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)


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

tmat · 2018-01-10T18:55:11Z

src/Interactive/Features/Interactive/Core/InteractiveHost.cs

-            }
+                if (disposing)
+                {
+                    GC.SuppressFinalize(this);


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

tmat · 2018-01-10T18:57:38Z

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


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

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

ivanbasov · 2018-01-16T19:25:46Z

@tmat, could you please review? #Resolved

tmat · 2018-01-17T18:33:07Z

src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs

-                            if (!_disposing)
+                        if (!_disposing)
+                        {
+                            using (await _disposeSemaphore.DisposableWaitAsync().ConfigureAwait(false))


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

tmat · 2018-01-17T18:35:56Z

src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs

+                if (Interlocked.Exchange(ref _processExitHandling, ProcessExitHandled) == ProcessExitHooked)
+                {
+                    Process.Exited -= ProcessExitedHandler;
+                }


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

tmat · 2018-01-17T18:37:40Z

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 _disposeSemaphore = new SemaphoreSlim(initialCount: 1);


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)

tmat · 2018-01-17T18:40:11Z

src/Interactive/Features/Interactive/Core/InteractiveHost.cs

+            {
+                semaphore.Wait();
+                semaphore.Dispose();
+            }


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

tmat · 2018-01-18T01:01:02Z

src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs

-                            {
-                                await _host.OnProcessExited(Process).ConfigureAwait(false);
-                            }
+                        if (!_disposing && _host != null)


_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)

tmat · 2018-01-18T01:20:52Z

src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs

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



object _disposeLock = new object(); #Resolved

tmat · 2018-01-18T01:22:20Z

src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs

                {
-                    try
+                    if (Interlocked.Exchange(ref _processExitHandling, ProcessExitHandled) == ProcessExitHooked)


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

tmat · 2018-01-18T01:23:35Z

src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs

+                        // 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();
+                    }
+                }


lock (_disposeLock) { _disposing = true; if (_processExitHandling == ProcessExitHooked) { Process.Exited -= ProcessExitedHandler; _processExitHandling = ProcessExitHandled; } } ``` #Resolved

tmat · 2018-01-18T01:24:50Z

src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs

+                if (Interlocked.Exchange(ref _processExitHandling, ProcessExitHooked) == 0)
+                {
+                    Process.Exited += ProcessExitedHandler;
+                }


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

tmat · 2018-01-18T02:15:39Z

src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs

@@ -116,34 +126,50 @@ internal void Dispose(bool joinThreads)
                // set _disposing so that we don't attempt restart the host anymore:
                _disposing = true;


_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)

tmat · 2018-01-18T02:19:38Z

src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs

                        {
-                            Process.Exited -= localHandler;
-
+                            Process.Exited -= ProcessExitedHandler;
                            if (!_disposing)


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

ivanbasov · 2018-01-23T21:00:29Z

@tmat, @jinujoseph, @jasonmalinowski, please review. I would like to push this fix into 15.6 #Resolved

tmat · 2018-01-23T21:07:18Z

@ivanbasov Does it meet the bar for escrow? I think moving it to 15.7 would be ok. #Resolved

ivanbasov · 2018-01-24T00:07:07Z

@tmat, could you please review? I hope I addressed all the comments. Thanks! #Resolved

tmat · 2018-01-24T00:22:23Z

src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs


            // 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 int _processExitHandling;  // set to ProcessExitHandled on dispose


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)

tmat

tmat · 2018-01-24T00:46:03Z

src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs

-                            }
+                            Process.Exited -= ProcessExitedHandler;
+                            _processExitHandlerStatus = ProcessExitHandlerStatus.Handled;
+                            // Should set _processExitHandling before calling OnProcessExited to avoid deadlocks.


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

nit: wrong name #Resolved

ivanbasov · 2018-01-24T00:58:28Z

Tagging @MattGertz to approve merge into dev15.7.x

ivanbasov · 2018-01-25T22:34:33Z

@jasonmalinowski please unblock #Resolved

jasonmalinowski

@ivanbasov I admit I still have some questions. I think there's some room for a better comment, maybe?

jasonmalinowski · 2018-01-25T22:48:25Z

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


Why not just call the public overload? #Resolved

jasonmalinowski · 2018-01-25T23:04:33Z

src/Interactive/Features/Interactive/Core/InteractiveHost.RemoteService.cs

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


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)

ivanbasov added Area-Interactive Tenet-Reliability Customer telemetry indicates that the product is failing in a crash/hang/dataloss manner. labels Jan 5, 2018

ivanbasov requested review from tmat, jasonmalinowski and a team January 5, 2018 00:18

jasonmalinowski reviewed Jan 5, 2018

View reviewed changes

jasonmalinowski requested changes Jan 5, 2018

View reviewed changes

tmat reviewed Jan 8, 2018

View reviewed changes

tmat requested changes Jan 8, 2018

View reviewed changes

tmat reviewed Jan 10, 2018

View reviewed changes

tmat reviewed Jan 17, 2018

View reviewed changes

tmat reviewed Jan 18, 2018

View reviewed changes

ivanbasov force-pushed the interactive branch from 3689744 to ddf5bff Compare January 19, 2018 00:07

ivanbasov added PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. and removed PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Jan 19, 2018

ivanbasov added 15 commits January 23, 2018 14:14

code review feedback

d67d272

fixing incomplete commit

0262e8b

code review feedback

cb57d2d

code review feedback

a3fce2b

code review feedback

e625b6d

code review feedback

91a8810

code review feedback

bb185c7

rebase

740a696

experiment dotnet#1

2977818

experiment dotnet#2

eb87327

experiment dotnet#3

30bd67e

Experiment dotnet#4

e299bf8

semaphore init corrected

9abe607

resolving a deadlock

ac9e071

feedback

68dfd2d

ivanbasov changed the base branch from master to dev15.7.x January 23, 2018 22:14

ivanbasov force-pushed the interactive branch from 14f365e to 68dfd2d Compare January 23, 2018 22:14

tmat reviewed Jan 24, 2018

View reviewed changes

feedback

49f7bf1

tmat approved these changes Jan 24, 2018

View reviewed changes

tmat reviewed Jan 24, 2018

View reviewed changes

comment correction

2243d2a

MattGertz added the Approved to merge label Jan 24, 2018

jasonmalinowski approved these changes Jan 25, 2018

View reviewed changes

code review feedback

5650219

ivanbasov merged commit 958c99e into dotnet:dev15.7.x Jan 26, 2018

ivanbasov deleted the interactive branch January 26, 2018 00:51

		@@ -116,34 +126,50 @@ internal void Dispose(bool joinThreads)
		// set _disposing so that we don't attempt restart the host anymore:
		_disposing = true;

unregister onExit event handler on disposing interactive host #24058

unregister onExit event handler on disposing interactive host #24058

Conversation

ivanbasov commented Jan 5, 2018 • edited Loading

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

How was the bug found?

jasonmalinowski Jan 5, 2018 • edited by ivanbasov Loading

Choose a reason for hiding this comment

jasonmalinowski left a comment

Choose a reason for hiding this comment

tmat Jan 8, 2018 • edited by ivanbasov Loading

Choose a reason for hiding this comment

tmat left a comment

Choose a reason for hiding this comment

tmat Jan 10, 2018 • edited by ivanbasov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

tmat Jan 10, 2018 • edited by ivanbasov Loading

Choose a reason for hiding this comment

tmat Jan 10, 2018 • edited by ivanbasov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Jan 10, 2018 • edited by ivanbasov Loading

Choose a reason for hiding this comment

tmat Jan 10, 2018 • edited by ivanbasov Loading

Choose a reason for hiding this comment

tmat Jan 10, 2018 • edited by ivanbasov Loading

Choose a reason for hiding this comment

ivanbasov commented Jan 16, 2018 • edited Loading

tmat Jan 17, 2018 • edited by ivanbasov Loading

Choose a reason for hiding this comment

tmat Jan 17, 2018 • edited by ivanbasov Loading

Choose a reason for hiding this comment

tmat Jan 17, 2018 • edited by ivanbasov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Jan 17, 2018 • edited by ivanbasov Loading

Choose a reason for hiding this comment

tmat Jan 18, 2018 • edited by ivanbasov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Jan 18, 2018 • edited by ivanbasov Loading

Choose a reason for hiding this comment

tmat Jan 18, 2018 • edited by ivanbasov Loading

Choose a reason for hiding this comment

tmat Jan 18, 2018 • edited by ivanbasov Loading

Choose a reason for hiding this comment

tmat Jan 18, 2018 • edited by ivanbasov Loading

Choose a reason for hiding this comment

tmat Jan 18, 2018 • edited by ivanbasov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Jan 18, 2018 • edited by ivanbasov Loading

Choose a reason for hiding this comment

ivanbasov commented Jan 23, 2018 • edited Loading

tmat commented Jan 23, 2018 • edited by ivanbasov Loading

ivanbasov commented Jan 24, 2018 • edited Loading

tmat Jan 24, 2018 • edited by ivanbasov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat left a comment

Choose a reason for hiding this comment

tmat Jan 24, 2018 • edited by ivanbasov Loading

Choose a reason for hiding this comment

ivanbasov commented Jan 24, 2018

ivanbasov commented Jan 25, 2018 • edited Loading

jasonmalinowski left a comment

Choose a reason for hiding this comment

jasonmalinowski Jan 25, 2018 • edited by ivanbasov Loading

Choose a reason for hiding this comment

jasonmalinowski Jan 25, 2018 • edited by ivanbasov Loading

ivanbasov commented Jan 5, 2018 •

edited

Loading

jasonmalinowski Jan 5, 2018 •

edited by ivanbasov

Loading

tmat Jan 8, 2018 •

edited by ivanbasov

Loading

tmat Jan 10, 2018 •

edited by ivanbasov

Loading

tmat Jan 10, 2018 •

edited

Loading

tmat Jan 10, 2018 •

edited by ivanbasov

Loading

tmat Jan 10, 2018 •

edited by ivanbasov

Loading

tmat Jan 10, 2018 •

edited by ivanbasov

Loading

tmat Jan 10, 2018 •

edited by ivanbasov

Loading

tmat Jan 10, 2018 •

edited by ivanbasov

Loading

ivanbasov commented Jan 16, 2018 •

edited

Loading

tmat Jan 17, 2018 •

edited by ivanbasov

Loading

tmat Jan 17, 2018 •

edited by ivanbasov

Loading

tmat Jan 17, 2018 •

edited by ivanbasov

Loading

tmat Jan 17, 2018 •

edited by ivanbasov

Loading

tmat Jan 18, 2018 •

edited by ivanbasov

Loading

tmat Jan 18, 2018 •

edited by ivanbasov

Loading

tmat Jan 18, 2018 •

edited by ivanbasov

Loading

tmat Jan 18, 2018 •

edited by ivanbasov

Loading

tmat Jan 18, 2018 •

edited by ivanbasov

Loading

tmat Jan 18, 2018 •

edited by ivanbasov

Loading

tmat Jan 18, 2018 •

edited by ivanbasov

Loading

ivanbasov commented Jan 23, 2018 •

edited

Loading

tmat commented Jan 23, 2018 •

edited by ivanbasov

Loading

ivanbasov commented Jan 24, 2018 •

edited

Loading

tmat Jan 24, 2018 •

edited by ivanbasov

Loading

tmat Jan 24, 2018 •

edited by ivanbasov

Loading

ivanbasov commented Jan 25, 2018 •

edited

Loading

jasonmalinowski Jan 25, 2018 •

edited by ivanbasov

Loading

jasonmalinowski Jan 25, 2018 •

edited by ivanbasov

Loading

jasonmalinowski Jan 25, 2018 •

edited by ivanbasov

Loading