Merge pull request #15110 from heejaechang/checkcancel

fixed 2 known OOP issues

src/VisualStudio/Core/Next/Remote/ServiceHubRemoteHostClient.cs

@@ -4,6 +4,7 @@
 using System.IO;
 using System.Threading;
 using System.Threading.Tasks;
+using Microsoft.CodeAnalysis.ErrorReporting;
 using Microsoft.CodeAnalysis.Execution;
 using Microsoft.CodeAnalysis.Internal.Log;
 using Microsoft.CodeAnalysis.Remote;
@@ -105,16 +106,33 @@ private void OnRpcDisconnected(object sender, JsonRpcDisconnectedEventArgs e)
 
         private static async Task<Stream> RequestServiceAsync(HubClient client, string serviceName, string hostGroup, CancellationToken cancellationToken = default(CancellationToken))
         {
-            // we can remove these once whole system moved to new servicehub API
-            try
-            {
-                var descriptor = new ServiceDescriptor(serviceName) { HostGroup = new HostGroup(hostGroup) };
-                return await client.RequestServiceAsync(descriptor, cancellationToken).ConfigureAwait(false);
-            }
-            catch
+            const int max_retry = 10;
+            const int retry_delayInMS = 50;
+
+            // call to get service can fail due to this bug - devdiv#288961
+            // until root cause is fixed, we decide to have retry rather than fail right away
+            for (var i = 0; i < max_retry; i++)
             {
-                return await client.RequestServiceAsync(serviceName, cancellationToken).ConfigureAwait(false);
+                cancellationToken.ThrowIfCancellationRequested();
+
+                try
+                {
+                    var descriptor = new ServiceDescriptor(serviceName) { HostGroup = new HostGroup(hostGroup) };
+                    return await client.RequestServiceAsync(descriptor, cancellationToken).ConfigureAwait(false);
+                }
+                catch (RemoteInvocationException ex)
+                {
+                    // RequestServiceAsync should never fail unless service itself is actually broken.
+                    // right now, we know only 1 case where it can randomly fail. but there might be more cases so 
+                    // adding non fatal watson here.
+                    FatalError.ReportWithoutCrash(ex);
+                }
+
+                // wait for retry_delayInMS before next try
+                await Task.Delay(retry_delayInMS, cancellationToken).ConfigureAwait(false);
             }
+
+            return Contract.FailWithReturn<Stream>("Fail to get service. look FatalError.s_reportedException for more detail");
         }
     }
 }
\ No newline at end of file

src/Workspaces/Remote/ServiceHub/Shared/Extensions.cs

@@ -18,10 +18,10 @@ internal static partial class Extensions
             this JsonRpc rpc, string targetName, IEnumerable<object> arguments,
             Func<Stream, CancellationToken, Task> funcWithDirectStreamAsync, CancellationToken cancellationToken)
         {
-            try
+            using (var mergedCancellation = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken))
+            using (var stream = new ServerDirectStream())
             {
-                using (var mergedCancellation = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken))
-                using (var stream = new ServerDirectStream())
+                try
                 {
                     // send request by adding direct stream name to end of arguments
                     var task = rpc.InvokeAsync(targetName, arguments.Concat(stream.Name).ToArray());
@@ -38,11 +38,11 @@ internal static partial class Extensions
                     // wait task to finish
                     await task.ConfigureAwait(false);
                 }
-            }
-            catch (Exception ex) when (IsCancelled(ex))
-            {
-                cancellationToken.ThrowIfCancellationRequested();
-                throw;
+                catch (Exception ex) when (ReportUnlessCanceled(ex, mergedCancellation.Token))
+                {
+                    cancellationToken.ThrowIfCancellationRequested();
+                    throw;
+                }
             }
         }
 
@@ -50,10 +50,10 @@ internal static partial class Extensions
             this JsonRpc rpc, string targetName, IEnumerable<object> arguments,
             Func<Stream, CancellationToken, Task<T>> funcWithDirectStreamAsync, CancellationToken cancellationToken)
         {
-            try
+            using (var mergedCancellation = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken))
+            using (var stream = new ServerDirectStream())
             {
-                using (var mergedCancellation = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken))
-                using (var stream = new ServerDirectStream())
+                try
                 {
                     // send request to asset source
                     var task = rpc.InvokeAsync(targetName, arguments.Concat(stream.Name).ToArray());
@@ -72,11 +72,11 @@ internal static partial class Extensions
 
                     return result;
                 }
-            }
-            catch (Exception ex) when (IsCancelled(ex))
-            {
-                cancellationToken.ThrowIfCancellationRequested();
-                throw;
+                catch (Exception ex) when (ReportUnlessCanceled(ex, mergedCancellation.Token))
+                {
+                    cancellationToken.ThrowIfCancellationRequested();
+                    throw;
+                }
             }
         }
 
@@ -84,10 +84,10 @@ internal static partial class Extensions
             this JsonRpc rpc, string targetName, IEnumerable<object> arguments,
             Action<Stream, CancellationToken> actionWithDirectStream, CancellationToken cancellationToken)
         {
-            try
+            using (var mergedCancellation = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken))
+            using (var stream = new ServerDirectStream())
             {
-                using (var mergedCancellation = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken))
-                using (var stream = new ServerDirectStream())
+                try
                 {
                     // send request by adding direct stream name to end of arguments
                     var task = rpc.InvokeAsync(targetName, arguments.Concat(stream.Name).ToArray());
@@ -104,11 +104,11 @@ internal static partial class Extensions
                     // wait task to finish
                     await task.ConfigureAwait(false);
                 }
-            }
-            catch (Exception ex) when (IsCancelled(ex))
-            {
-                cancellationToken.ThrowIfCancellationRequested();
-                throw;
+                catch (Exception ex) when (ReportUnlessCanceled(ex, mergedCancellation.Token))
+                {
+                    cancellationToken.ThrowIfCancellationRequested();
+                    throw;
+                }
             }
         }
 
@@ -116,10 +116,10 @@ internal static partial class Extensions
             this JsonRpc rpc, string targetName, IEnumerable<object> arguments,
             Func<Stream, CancellationToken, T> funcWithDirectStream, CancellationToken cancellationToken)
         {
-            try
+            using (var mergedCancellation = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken))
+            using (var stream = new ServerDirectStream())
             {
-                using (var mergedCancellation = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken))
-                using (var stream = new ServerDirectStream())
+                try
                 {
                     // send request to asset source
                     var task = rpc.InvokeAsync(targetName, arguments.Concat(stream.Name).ToArray());
@@ -138,25 +138,27 @@ internal static partial class Extensions
 
                     return result;
                 }
-            }
-            catch (Exception ex) when (IsCancelled(ex))
-            {
-                cancellationToken.ThrowIfCancellationRequested();
-                throw;
+                catch (Exception ex) when (ReportUnlessCanceled(ex, mergedCancellation.Token))
+                {
+                    cancellationToken.ThrowIfCancellationRequested();
+                    throw;
+                }
             }
         }
 
-        private static bool IsCancelled(Exception ex)
+        private static bool ReportUnlessCanceled(Exception ex, CancellationToken token)
         {
-            // object disposed exception can be thrown from StreamJsonRpc if JsonRpc is disposed in the middle of read/write.
-            // the way we added cancellation support to the JsonRpc which doesn't support cancellation natively
-            // can cause this exception to happen. newer version supports cancellation token natively, but
-            // we can't use it now, so we will catch object disposed exception and check cancellation token
-            if (ex is ObjectDisposedException || ex is OperationCanceledException)
+            // check whether we are in cancellation mode
+            if (token.IsCancellationRequested)
             {
+                // we are under cancellation, we don't care what the exception is.
+                // due to the way we do cancellation (forcefully closing connection in the middle of reading/writing)
+                // various exceptions can be thrown. for example, if we close our own named pipe stream in the middle of
+                // object reader/writer using it, we could get invalid operation exception or invalid cast exception.
                 return true;
             }
 
+            // unexpected exception case. crash VS
             return FatalError.Report(ex);
         }