Merge pull request #2140 from OmniSharp/bugfix/config-fail

Do not crash on startup when configuration is invalid

src/OmniSharp.Host/ConfigurationBuilder.cs

@@ -1,55 +1,55 @@
 using System;
 using System.Collections.Generic;
+using System.Linq;
 using Microsoft.Extensions.Configuration;
 using Microsoft.Extensions.FileProviders;
 using OmniSharp.Internal;
 using OmniSharp.Utilities;
 
 namespace OmniSharp
 {
-    public class ConfigurationBuilder : IConfigurationBuilder
+    public class ConfigurationBuilder
     {
         private readonly IOmniSharpEnvironment _environment;
-        private readonly IConfigurationBuilder _builder;
 
         public ConfigurationBuilder(IOmniSharpEnvironment environment)
         {
             _environment = environment;
-            _builder = new Microsoft.Extensions.Configuration.ConfigurationBuilder()
-                .SetBasePath(AppContext.BaseDirectory);
         }
 
-        public IConfigurationBuilder Add(IConfigurationSource source)
+        public ConfigurationResult Build(Action<IConfigurationBuilder> additionalSetup = null)
         {
-            _builder.Add(source);
-            return this;
-        }
-
-        public IConfigurationRoot Build()
-        {
-            var configBuilder = new Microsoft.Extensions.Configuration.ConfigurationBuilder()
-                .SetBasePath(AppContext.BaseDirectory)
-                .AddEnvironmentVariables("OMNISHARP_");
-
-            if (_environment.AdditionalArguments?.Length > 0)
+            try
             {
-                configBuilder.AddCommandLine(_environment.AdditionalArguments);
+                var configBuilder = new Microsoft.Extensions.Configuration.ConfigurationBuilder()
+                    .SetBasePath(AppContext.BaseDirectory)
+                    .AddEnvironmentVariables("OMNISHARP_");
+
+                if (_environment.AdditionalArguments?.Length > 0)
+                {
+                    configBuilder.AddCommandLine(_environment.AdditionalArguments);
+                }
+
+                // Use the global omnisharp config if there's any in the shared path
+                configBuilder.CreateAndAddGlobalOptionsFile(_environment);
+
+                // Use the local omnisharp config if there's any in the root path
+                configBuilder.AddJsonFile(
+                    new PhysicalFileProvider(_environment.TargetDirectory).WrapForPolling(),
+                    Constants.OptionsFile,
+                    optional: true,
+                    reloadOnChange: true);
+
+                // bootstrap additional host configuration at the end
+                additionalSetup?.Invoke(configBuilder);
+
+                var config = configBuilder.Build();
+                return new ConfigurationResult(config);
+            }
+            catch (Exception ex)
+            {
+                return new ConfigurationResult(ex);
             }
-
-            // Use the global omnisharp config if there's any in the shared path
-            configBuilder.CreateAndAddGlobalOptionsFile(_environment);
-
-            // Use the local omnisharp config if there's any in the root path
-            configBuilder.AddJsonFile(
-                new PhysicalFileProvider(_environment.TargetDirectory).WrapForPolling(),
-                Constants.OptionsFile,
-                optional: true,
-                reloadOnChange: true);
-
-            return configBuilder.Build();
         }
-
-        public IDictionary<string, object> Properties => _builder.Properties;
-        public IList<IConfigurationSource> Sources => _builder.Sources;
     }
 }

src/OmniSharp.Host/ConfigurationResult.cs

@@ -0,0 +1,25 @@
+using System;
+using Microsoft.Extensions.Configuration;
+
+namespace OmniSharp
+{
+    public class ConfigurationResult
+    {
+        public ConfigurationResult(IConfigurationRoot configuration)
+        {
+            Configuration = configuration;
+        }
+
+        public ConfigurationResult(Exception exception)
+        {
+            Exception = exception;
+            Configuration = new ConfigurationRoot(Array.Empty<IConfigurationProvider>());
+        }
+
+        public IConfigurationRoot Configuration { get; }
+
+        public Exception Exception { get; }
+
+        public bool HasError() => Exception != null;
+    }
+}

src/OmniSharp.Http/Startup.cs

@@ -32,8 +32,8 @@ public Startup(IOmniSharpEnvironment environment, IEventEmitter eventEmitter, Pl
 
         public IServiceProvider ConfigureServices(IServiceCollection services)
         {
-            var configuration = new ConfigurationBuilder(_environment).Build();
-            var serviceProvider = CompositionHostBuilder.CreateDefaultServiceProvider(_environment, configuration, _eventEmitter, services,
+            var configurationResult = new ConfigurationBuilder(_environment).Build();
+            var serviceProvider = CompositionHostBuilder.CreateDefaultServiceProvider(_environment, configurationResult.Configuration, _eventEmitter, services,
                 configureLogging: builder =>
                 {
                     builder.AddConsole();
@@ -56,6 +56,12 @@ public IServiceProvider ConfigureServices(IServiceCollection services)
 
             var loggerFactory = serviceProvider.GetRequiredService<ILoggerFactory>();
             var logger = loggerFactory.CreateLogger<Startup>();
+
+            if (configurationResult.HasError())
+            {
+                logger.LogError(configurationResult.Exception, "There was an error when reading the OmniSharp configuration, starting with the default options.");
+            }
+
             var assemblyLoader = serviceProvider.GetRequiredService<IAssemblyLoader>();
             _compositionHost = new CompositionHostBuilder(serviceProvider)
                 .WithOmniSharpAssemblies()

src/OmniSharp.LanguageServerProtocol/LanguageServerHost.cs

@@ -202,24 +202,23 @@ private static (IServiceProvider serviceProvider, CompositionHost compositionHos
                 application.LogLevel < logLevel ? application.LogLevel : logLevel,
                 application.OtherArgs.ToArray());
 
-            var configurationRoot = new Microsoft.Extensions.Configuration.ConfigurationBuilder()
-                    .AddConfiguration(new ConfigurationBuilder(environment).Build())
-                    .AddConfiguration(server.Configuration.GetSection("csharp"))
-                    .AddConfiguration(server.Configuration.GetSection("omnisharp"))
-                    .Build()
-                ;
-
+            var configurationResult = new ConfigurationBuilder(environment).Build(b =>
+                b.AddConfiguration(server.Configuration.GetSection("csharp")).AddConfiguration(server.Configuration.GetSection("omnisharp")));
             var eventEmitter = new LanguageServerEventEmitter(server);
 
             services.AddSingleton(server)
                 .AddSingleton<ILanguageServerFacade>(server);
 
             var serviceProvider =
-                CompositionHostBuilder.CreateDefaultServiceProvider(environment, configurationRoot, eventEmitter,
+                CompositionHostBuilder.CreateDefaultServiceProvider(environment, configurationResult.Configuration, eventEmitter,
                     services, GetLogBuilderAction(configureLogging, environment.LogLevel));
 
             var loggerFactory = serviceProvider.GetService<ILoggerFactory>();
             var logger = loggerFactory.CreateLogger<LanguageServerHost>();
+            if (configurationResult.HasError())
+            {
+                logger.LogError(configurationResult.Exception, "There was an error when reading the OmniSharp configuration, starting with the default options.");
+            }
 
             var options = serviceProvider.GetRequiredService<IOptionsMonitor<OmniSharpOptions>>();
             var plugins = application.CreatePluginAssemblies(options.CurrentValue, environment);

src/OmniSharp.Stdio.Driver/Program.cs

@@ -51,9 +51,9 @@ static int Main(string[] args) => HostHelpers.Start(() =>
 
                     var environment = application.CreateEnvironment();
                     Configuration.ZeroBasedIndices = application.ZeroBasedIndices;
-                    var configuration = new ConfigurationBuilder(environment).Build();
+                    var configurationResult = new ConfigurationBuilder(environment).Build();
                     var writer = new SharedTextWriter(output);
-                    var serviceProvider = CompositionHostBuilder.CreateDefaultServiceProvider(environment, configuration, new StdioEventEmitter(writer),
+                    var serviceProvider = CompositionHostBuilder.CreateDefaultServiceProvider(environment, configurationResult.Configuration, new StdioEventEmitter(writer),
                         configureLogging: builder => builder.AddStdio(writer));
 
                     var loggerFactory = serviceProvider.GetRequiredService<ILoggerFactory>();
@@ -63,6 +63,10 @@ static int Main(string[] args) => HostHelpers.Start(() =>
                     var plugins = application.CreatePluginAssemblies(options.CurrentValue, environment);
 
                     var logger = loggerFactory.CreateLogger<Program>();
+                    if (configurationResult.HasError())
+                    {
+                        logger.LogError(configurationResult.Exception, "There was an error when reading the OmniSharp configuration, starting with the default options.");
+                    }
                     var compositionHostBuilder = new CompositionHostBuilder(serviceProvider)
                         .WithOmniSharpAssemblies()
                         .WithAssemblies(assemblyLoader.LoadByAssemblyNameOrPath(logger, plugins.AssemblyNames).ToArray());

tests/OmniSharp.Tests/ConfigurationBuilderFacts.cs

@@ -0,0 +1,65 @@
+using System;
+using System.Collections.Generic;
+using Microsoft.Extensions.Configuration;
+using OmniSharp.Services;
+using Xunit;
+
+namespace OmniSharp.Tests
+{
+    public class ConfigurationBuilderFacts
+    {
+        [Fact]
+        public void CustomConfigCanBeAdded()
+        {
+            var env = new OmniSharpEnvironment();
+            var builder = new ConfigurationBuilder(env);
+            var result = builder.Build(c => c.AddInMemoryCollection(new Dictionary<string, string> { { "key", "value" } }));
+
+            Assert.False(result.HasError());
+            Assert.Equal("value", result.Configuration["key"]);
+        }
+
+        [Fact]
+        public void EnvironmentVariableCanBeRead()
+        {
+            var tempValue = Guid.NewGuid().ToString("d");
+            try
+            {
+                Environment.SetEnvironmentVariable("OMNISHARP_testValue", tempValue);
+                var env = new OmniSharpEnvironment();
+                var builder = new ConfigurationBuilder(env);
+                var result = builder.Build(c => c.AddInMemoryCollection());
+
+                Assert.False(result.HasError());
+                Assert.Equal(tempValue, result.Configuration["testValue"]);
+            }
+            finally
+            {
+                Environment.SetEnvironmentVariable("OMNISHARP_testValue", null);
+            }
+        }
+
+        [Fact]
+        public void FileArgsCanBeRead()
+        {
+            var env = new OmniSharpEnvironment(additionalArguments: new string[] { "key:nestedKey=value" });
+            var builder = new ConfigurationBuilder(env);
+            var result = builder.Build();
+
+            Assert.False(result.HasError());
+            Assert.Equal("value", result.Configuration["key:nestedKey"]);
+        }
+
+        [Fact]
+        public void DoesNotCrashOnException()
+        {
+            var env = new OmniSharpEnvironment();
+            var builder = new ConfigurationBuilder(env);
+            var result = builder.Build(c => throw new Exception("bad thing happened"));
+
+            Assert.True(result.HasError());
+            Assert.NotNull(result.Configuration);
+            Assert.Empty(result.Configuration.AsEnumerable());
+        }
+    }
+}