From 3009caa6e84858e0f9abc2ae38ae4b4990d27d1e Mon Sep 17 00:00:00 2001 From: Heejae Chang Date: Thu, 24 Aug 2017 23:01:43 -0700 Subject: [PATCH 1/2] added retry to InvokeAsync as well for OOP start up. --- .../Next/Remote/ServiceHubRemoteHostClient.cs | 139 +++++++++++------- .../Services/ServiceHubServicesTests.cs | 2 +- .../Portable/Remote/IRemoteHostService.cs | 3 +- .../ServiceHub/Services/RemoteHostService.cs | 19 ++- 4 files changed, 104 insertions(+), 59 deletions(-) diff --git a/src/VisualStudio/Core/Next/Remote/ServiceHubRemoteHostClient.cs b/src/VisualStudio/Core/Next/Remote/ServiceHubRemoteHostClient.cs index 481bce7bc40b5..d920ab0af642c 100644 --- a/src/VisualStudio/Core/Next/Remote/ServiceHubRemoteHostClient.cs +++ b/src/VisualStudio/Core/Next/Remote/ServiceHubRemoteHostClient.cs @@ -36,23 +36,14 @@ public static async Task CreateAsync( { using (Logger.LogBlock(FunctionId.ServiceHubRemoteHostClient_CreateAsync, cancellationToken)) { - // let each client to have unique id so that we can distinguish different clients when service is restarted - var currentInstanceId = Interlocked.Add(ref s_instanceId, 1); - var primary = new HubClient("ManagedLanguage.IDE.RemoteHostClient"); - var current = $"VS ({Process.GetCurrentProcess().Id}) ({currentInstanceId})"; - - var hostGroup = new HostGroup(current); var timeout = TimeSpan.FromMilliseconds(workspace.Options.GetOption(RemoteHostOptions.RequestServiceTimeoutInMS)); - var remoteHostStream = await RequestServiceAsync(primary, WellKnownRemoteHostServices.RemoteHostService, hostGroup, timeout, cancellationToken).ConfigureAwait(false); - - var instance = new ServiceHubRemoteHostClient(workspace, primary, hostGroup, remoteHostStream); - - // make sure connection is done right - var host = await instance._rpc.InvokeAsync(nameof(IRemoteHostService.Connect), current, TelemetryService.DefaultSession.SerializeSettings()).ConfigureAwait(false); - // TODO: change this to non fatal watson and make VS to use inproc implementation - Contract.ThrowIfFalse(host == current.ToString()); + // Retry (with timeout) until we can connect to RemoteHost (service hub process). + // we are seeing cases where we failed to connect to service hub process when a machine is under heavy load. + // (see https://devdiv.visualstudio.com/DevDiv/_workitems/edit/481103 as one of example) + var instance = await RetryRemoteCallAsync( + () => CreateWorkerAsync(workspace, primary, timeout, cancellationToken), timeout, cancellationToken).ConfigureAwait(false); instance.Connected(); @@ -65,6 +56,43 @@ public static async Task CreateAsync( } } + public static async Task CreateWorkerAsync(Workspace workspace, HubClient primary, TimeSpan timeout, CancellationToken cancellationToken) + { + ServiceHubRemoteHostClient client = null; + try + { + // let each client to have unique id so that we can distinguish different clients when service is restarted + var currentInstanceId = Interlocked.Add(ref s_instanceId, 1); + + var current = $"VS ({Process.GetCurrentProcess().Id}) ({currentInstanceId})"; + + var hostGroup = new HostGroup(current); + var remoteHostStream = await RequestServiceAsync( + primary, WellKnownRemoteHostServices.RemoteHostService, hostGroup, timeout, cancellationToken).ConfigureAwait(false); + + client = new ServiceHubRemoteHostClient(workspace, primary, hostGroup, remoteHostStream); + + await client._rpc.InvokeWithCancellationAsync( + nameof(IRemoteHostService.Connect), + new object[] { current, TelemetryService.DefaultSession.SerializeSettings() }, + cancellationToken).ConfigureAwait(false); + + return client; + } + catch (Exception ex) + { + // make sure we shutdown client if initializing client has failed. + client?.Shutdown(); + + // translate to our own cancellation if it is raised. + cancellationToken.ThrowIfCancellationRequested(); + + // otherwise, report watson and throw original exception + WatsonReporter.Report("ServiceHub creation failed", ex, ReportDetailInfo); + throw; + } + } + private static async Task RegisterWorkspaceHostAsync(Workspace workspace, RemoteHostClient client) { var vsWorkspace = workspace as VisualStudioWorkspaceImpl; @@ -88,7 +116,7 @@ await Task.Factory.SafeStartNew(() => private ServiceHubRemoteHostClient( Workspace workspace, HubClient hubClient, HostGroup hostGroup, Stream stream) : - base(workspace) + base(workspace) { _hubClient = hubClient; _hostGroup = hostGroup; @@ -136,6 +164,40 @@ private void OnRpcDisconnected(object sender, JsonRpcDisconnectedEventArgs e) Disconnected(); } + /// + /// call and retry up to if the call throws + /// . any other exception from the call won't be handled here. + /// + private static async Task RetryRemoteCallAsync( + Func> funcAsync, + TimeSpan timeout, + CancellationToken cancellationToken) where TException : Exception + { + const int retry_delayInMS = 50; + + var start = DateTime.UtcNow; + while (DateTime.UtcNow - start < timeout) + { + cancellationToken.ThrowIfCancellationRequested(); + + try + { + return await funcAsync().ConfigureAwait(false); + } + catch (TException) + { + // throw cancellation token if operation is cancelled + cancellationToken.ThrowIfCancellationRequested(); + } + + // wait for retry_delayInMS before next try + await Task.Delay(retry_delayInMS, cancellationToken).ConfigureAwait(false); + } + + // operation timed out, more than we are willing to wait + throw new TimeoutException("RequestServiceAsync timed out"); + } + private static async Task RequestServiceAsync( HubClient client, string serviceName, @@ -156,7 +218,17 @@ private static async Task RequestServiceAsync( { try { - return await RequestServiceAsync(client, descriptor, timeout, cancellationToken).ConfigureAwait(false); + // we are wrapping HubClient.RequestServiceAsync since we can't control its internal timeout value ourselves. + // we have bug opened to track the issue. + // https://devdiv.visualstudio.com/DefaultCollection/DevDiv/Editor/_workitems?id=378757&fullScreen=false&_a=edit + + // retry on cancellation token since HubClient will throw its own cancellation token + // when it couldn't connect to service hub service for some reasons + // (ex, OOP process GC blocked and not responding to request) + return await RetryRemoteCallAsync( + () => client.RequestServiceAsync(descriptor, cancellationToken), + timeout, + cancellationToken).ConfigureAwait(false); } catch (RemoteInvocationException ex) { @@ -184,41 +256,6 @@ private static async Task RequestServiceAsync( throw ExceptionUtilities.Unreachable; } - private static async Task RequestServiceAsync(HubClient client, ServiceDescriptor descriptor, TimeSpan timeout, CancellationToken cancellationToken = default(CancellationToken)) - { - // we are wrapping HubClient.RequestServiceAsync since we can't control its internal timeout value ourselves. - // we have bug opened to track the issue. - // https://devdiv.visualstudio.com/DefaultCollection/DevDiv/Editor/_workitems?id=378757&fullScreen=false&_a=edit - const int retry_delayInMS = 50; - - var start = DateTime.UtcNow; - while (start - DateTime.UtcNow < timeout) - { - cancellationToken.ThrowIfCancellationRequested(); - - try - { - return await client.RequestServiceAsync(descriptor, cancellationToken).ConfigureAwait(false); - } - catch (OperationCanceledException) - { - // if it is our own cancellation token, then rethrow - // otherwise, let us retry. - // - // we do this since HubClient itself can throw its own cancellation token - // when it couldn't connect to service hub service for some reasons - // (ex, OOP process GC blocked and not responding to request) - cancellationToken.ThrowIfCancellationRequested(); - } - - // wait for retry_delayInMS before next try - await Task.Delay(retry_delayInMS, cancellationToken).ConfigureAwait(false); - } - - // request service to HubClient timed out, more than we are willing to wait - throw new TimeoutException("RequestServiceAsync timed out"); - } - private static int ReportDetailInfo(IFaultUtility faultUtility) { // 0 means send watson, otherwise, cancel watson diff --git a/src/VisualStudio/Core/Test.Next/Services/ServiceHubServicesTests.cs b/src/VisualStudio/Core/Test.Next/Services/ServiceHubServicesTests.cs index 4830f76dd3ef7..8bdea4cf82e8e 100644 --- a/src/VisualStudio/Core/Test.Next/Services/ServiceHubServicesTests.cs +++ b/src/VisualStudio/Core/Test.Next/Services/ServiceHubServicesTests.cs @@ -37,7 +37,7 @@ public void TestRemoteHostConnect() var remoteHostService = CreateService(); var input = "Test"; - var output = remoteHostService.Connect(input, serializedSession: null); + var output = remoteHostService.Connect(input, serializedSession: null, cancellationToken: CancellationToken.None); Assert.Equal(input, output); } diff --git a/src/Workspaces/Core/Portable/Remote/IRemoteHostService.cs b/src/Workspaces/Core/Portable/Remote/IRemoteHostService.cs index 593d45cdab0dd..fff3220fef9a3 100644 --- a/src/Workspaces/Core/Portable/Remote/IRemoteHostService.cs +++ b/src/Workspaces/Core/Portable/Remote/IRemoteHostService.cs @@ -1,12 +1,13 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System.Threading; using System.Threading.Tasks; namespace Microsoft.CodeAnalysis.Remote { internal interface IRemoteHostService { - string Connect(string host, string serializedSession); + string Connect(string host, string serializedSession, CancellationToken cancellationToken); Task SynchronizePrimaryWorkspaceAsync(Checksum checksum); Task SynchronizeGlobalAssetsAsync(Checksum[] checksums); diff --git a/src/Workspaces/Remote/ServiceHub/Services/RemoteHostService.cs b/src/Workspaces/Remote/ServiceHub/Services/RemoteHostService.cs index 3adee6306e184..2800ea45e0e87 100644 --- a/src/Workspaces/Remote/ServiceHub/Services/RemoteHostService.cs +++ b/src/Workspaces/Remote/ServiceHub/Services/RemoteHostService.cs @@ -41,11 +41,6 @@ static RemoteHostService() // we set up logger here RoslynLogger.SetLogger(new EtwLogger(GetLoggingChecker())); - // Set this process's priority BelowNormal. - // this should let us to freely try to use all resources possible without worrying about affecting - // host's work such as responsiveness or build. - Process.GetCurrentProcess().PriorityClass = ProcessPriorityClass.BelowNormal; - SetNativeDllSearchDirectories(); } @@ -56,8 +51,11 @@ public RemoteHostService(Stream stream, IServiceProvider serviceProvider) : Rpc.StartListening(); } - public string Connect(string host, string serializedSession) + public string Connect(string host, string serializedSession, CancellationToken cancellationToken) { + cancellationToken.ThrowIfCancellationRequested(); + + // this is called only once when Host (VS) started RemoteHost (OOP) _primaryInstance = InstanceId; var existing = Interlocked.CompareExchange(ref _host, host, null); @@ -72,6 +70,15 @@ public string Connect(string host, string serializedSession) // log telemetry that service hub started RoslynLogger.Log(FunctionId.RemoteHost_Connect, KeyValueLogMessage.Create(SetSessionInfo)); + // serializedSession will be null for testing + if (serializedSession != null) + { + // Set this process's priority BelowNormal. + // this should let us to freely try to use all resources possible without worrying about affecting + // host's work such as responsiveness or build. + Process.GetCurrentProcess().PriorityClass = ProcessPriorityClass.BelowNormal; + } + return _host; } From eb3f5baaaf8eb3c1bf43f32618ccc5e85782a070 Mon Sep 17 00:00:00 2001 From: Julien Couvreur Date: Thu, 31 Aug 2017 20:04:59 -0700 Subject: [PATCH 2/2] Silently treat IncludePrivateMembers as true when emitting regular assemblies (#21359) --- .../Test/Emit/Emit/CompilationEmitTests.cs | 37 +++++++++++++++++-- .../Core/Portable/Compilation/Compilation.cs | 5 ++- 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/src/Compilers/CSharp/Test/Emit/Emit/CompilationEmitTests.cs b/src/Compilers/CSharp/Test/Emit/Emit/CompilationEmitTests.cs index 2fb1b7842f307..91e86c1f290f5 100644 --- a/src/Compilers/CSharp/Test/Emit/Emit/CompilationEmitTests.cs +++ b/src/Compilers/CSharp/Test/Emit/Emit/CompilationEmitTests.cs @@ -1576,6 +1576,36 @@ public class PublicClass "System.Diagnostics.DebuggableAttribute" }, compWithReal.SourceModule.GetReferencedAssemblySymbols().Last().GetAttributes().Select(a => a.AttributeClass.ToTestDisplayString())); + // Verify metadata (types, members, attributes) of the regular assembly with IncludePrivateMembers accidentally set to false. + // Note this can happen because of binary clients compiled against old EmitOptions ctor which had IncludePrivateMembers=false by default. + // In this case, IncludePrivateMembers is silently set to true when emitting + // See https://github.com/dotnet/roslyn/issues/20873 + var emitRegularWithoutPrivateMembers = EmitOptions.Default.WithIncludePrivateMembers(false); + CompileAndVerify(comp, emitOptions: emitRegularWithoutPrivateMembers, verify: true); + + var realImage2 = comp.EmitToImageReference(emitRegularWithoutPrivateMembers); + var compWithReal2 = CreateCompilation("", references: new[] { MscorlibRef, realImage2 }, + options: TestOptions.DebugDll.WithMetadataImportOptions(MetadataImportOptions.All)); + AssertEx.Equal( + new[] { "", "<>f__AnonymousType0<j__TPar>", "PublicClass" }, + compWithReal2.SourceModule.GetReferencedAssemblySymbols().Last().GlobalNamespace.GetMembers().Select(m => m.ToDisplayString())); + + AssertEx.Equal( + new[] { "void PublicClass.PublicMethod()", "void PublicClass.PrivateMethod()", + "void PublicClass.ProtectedMethod()", "void PublicClass.InternalMethod()", + "void PublicClass.PublicEvent.add", "void PublicClass.PublicEvent.remove", + "void PublicClass.InternalEvent.add", "void PublicClass.InternalEvent.remove", + "PublicClass..ctor()", + "event System.Action PublicClass.PublicEvent", "event System.Action PublicClass.InternalEvent" }, + compWithReal2.GetMember("PublicClass").GetMembers() + .Select(m => m.ToTestDisplayString())); + + AssertEx.Equal( + new[] { "System.Runtime.CompilerServices.CompilationRelaxationsAttribute", + "System.Runtime.CompilerServices.RuntimeCompatibilityAttribute", + "System.Diagnostics.DebuggableAttribute" }, + compWithReal2.SourceModule.GetReferencedAssemblySymbols().Last().GetAttributes().Select(a => a.AttributeClass.ToTestDisplayString())); + // verify metadata (types, members, attributes) of the metadata-only assembly var emitMetadataOnly = EmitOptions.Default.WithEmitMetadataOnly(true); CompileAndVerify(comp, emitOptions: emitMetadataOnly, verify: true); @@ -1732,15 +1762,16 @@ public void IncludePrivateMembers_DisallowMetadataPeStream() } [Fact] - public void MustIncludePrivateMembersUnlessRefAssembly() + [WorkItem(20873, "https://github.com/dotnet/roslyn/issues/20873")] + public void IncludePrivateMembersSilentlyAssumedTrueWhenEmittingRegular() { CSharpCompilation comp = CreateCompilation("", references: new[] { MscorlibRef }, options: TestOptions.DebugDll.WithDeterministic(true)); using (var output = new MemoryStream()) { - Assert.Throws(() => comp.Emit(output, - options: EmitOptions.Default.WithIncludePrivateMembers(false))); + // no exception + _ = comp.Emit(output, options: EmitOptions.Default.WithIncludePrivateMembers(false)); } } diff --git a/src/Compilers/Core/Portable/Compilation/Compilation.cs b/src/Compilers/Core/Portable/Compilation/Compilation.cs index 3ea6eb9df3c7f..cacde7d19f7ab 100644 --- a/src/Compilers/Core/Portable/Compilation/Compilation.cs +++ b/src/Compilers/Core/Portable/Compilation/Compilation.cs @@ -2086,9 +2086,10 @@ public EmitResult Emit( throw new ArgumentException(CodeAnalysisResources.IncludingPrivateMembersUnexpectedWhenEmittingToMetadataPeStream, nameof(metadataPEStream)); } - if (metadataPEStream == null && options?.EmitMetadataOnly == false && options?.IncludePrivateMembers == false) + if (metadataPEStream == null && options?.EmitMetadataOnly == false) { - throw new ArgumentException(CodeAnalysisResources.MustIncludePrivateMembersUnlessRefAssembly, nameof(options.IncludePrivateMembers)); + // EmitOptions used to default to IncludePrivateMembers=false, so to preserve binary compatibility we silently correct that unless emitting regular assemblies + options = options.WithIncludePrivateMembers(true); } if (options?.DebugInformationFormat == DebugInformationFormat.Embedded &&