Merge pull request #20806 from CyrusNajmabadi/storateResiliency15.3

Be more resilient to external errors when trying to create our underlying persistent storage

src/VisualStudio/CSharp/Test/PersistentStorage/AbstractPersistentStorageTests.cs

@@ -401,15 +401,16 @@ protected Solution CreateOrOpenSolution(bool nullPaths = false)
             return solution;
         }
 
-        protected IPersistentStorage GetStorage(Solution solution)
+        internal IPersistentStorage GetStorage(
+            Solution solution, IPersistentStorageFaultInjector faultInjectorOpt = null)
         {
-            var storage = GetStorageService().GetStorage(solution);
+            var storage = GetStorageService(faultInjectorOpt).GetStorage(solution);
 
             Assert.NotEqual(NoOpPersistentStorage.Instance, storage);
             return storage;
         }
 
-        protected abstract IPersistentStorageService GetStorageService();
+        internal abstract IPersistentStorageService GetStorageService(IPersistentStorageFaultInjector faultInjector);
 
         protected Stream EncodeString(string text)
         {

src/VisualStudio/CSharp/Test/PersistentStorage/EsentPersistentStorageTests1.cs

@@ -12,7 +12,7 @@ namespace Microsoft.CodeAnalysis.UnitTests.WorkspaceServices
     /// </remarks>
     public class EsentPersistentStorageTests : AbstractPersistentStorageTests
     {
-        protected override IPersistentStorageService GetStorageService()
+        internal override IPersistentStorageService GetStorageService(IPersistentStorageFaultInjector faultInjector)
             => new EsentPersistentStorageService(_persistentEnabledOptionService, testing: true);
     }
 }
\ No newline at end of file

src/VisualStudio/CSharp/Test/PersistentStorage/SQLitePersistentStorageTests.cs

 // 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;
 using System.Linq;
 using System.Threading.Tasks;
 using Microsoft.CodeAnalysis.Host;
@@ -15,8 +16,8 @@ namespace Microsoft.CodeAnalysis.UnitTests.WorkspaceServices
     /// </remarks>
     public class SQLitePersistentStorageTests : AbstractPersistentStorageTests
     {
-        protected override IPersistentStorageService GetStorageService()
-            => new SQLitePersistentStorageService(_persistentEnabledOptionService, testing: true);
+        internal override IPersistentStorageService GetStorageService(IPersistentStorageFaultInjector faultInjector)
+            => new SQLitePersistentStorageService(_persistentEnabledOptionService, faultInjector);
 
         [Fact]
         public async Task TestNullFilePaths()
@@ -36,5 +37,56 @@ public async Task TestNullFilePaths()
                 Assert.Null(await storage.ReadStreamAsync(document, streamName));
             }
         }
+
+        [Fact]
+        public void TestCrashInNewConnection()
+        {
+            var solution = CreateOrOpenSolution(nullPaths: true);
+
+            var hitInjector = false;
+            var faultInjector = new PersistentStorageFaultInjector(
+                onNewConnection: () =>
+                {
+                    hitInjector = true;
+                    throw new Exception();
+                },
+                onFatalError: e => throw e);
+
+            using (var storage = GetStorageService(faultInjector).GetStorage(solution))
+            {
+                // Because instantiating hte connection will fail, we will not get back
+                // a working persistent storage.
+                Assert.IsType<NoOpPersistentStorage>(storage);
+            }
+
+            Assert.True(hitInjector);
+
+            // Ensure we don't get a crash due to SqlConnection's finalizer running.
+            for (var i = 0; i < 10; i++)
+            {
+                GC.Collect();
+                GC.WaitForPendingFinalizers();
+            }
+        }
+
+        private class PersistentStorageFaultInjector : IPersistentStorageFaultInjector
+        {
+            private readonly Action _onNewConnection;
+            private readonly Action<Exception> _onFatalError;
+
+            public PersistentStorageFaultInjector(
+                Action onNewConnection = null,
+                Action<Exception> onFatalError = null)
+            {
+                _onNewConnection = onNewConnection;
+                _onFatalError = onFatalError;
+            }
+
+            public void OnNewConnection()
+                => _onNewConnection?.Invoke();
+
+            public void OnFatalError(Exception ex)
+                => _onFatalError?.Invoke(ex);
+        }
     }
 }
\ No newline at end of file

src/Workspaces/Core/Desktop/Workspace/SQLite/Interop/SqlConnection.cs

@@ -4,6 +4,7 @@
 using System.Collections.Generic;
 using System.IO;
 using Microsoft.CodeAnalysis.ErrorReporting;
+using Microsoft.CodeAnalysis.Host;
 using Roslyn.Utilities;
 using SQLitePCL;
 
@@ -29,6 +30,11 @@ internal partial class SqlConnection
         /// </summary>
         private readonly sqlite3 _handle;
 
+        /// <summary>
+        /// For testing purposes to simulate failures during testing.
+        /// </summary>
+        private readonly IPersistentStorageFaultInjector _faultInjector;
+
         /// <summary>
         /// Our cache of prepared statements for given sql strings.
         /// </summary>
@@ -41,10 +47,11 @@ internal partial class SqlConnection
         /// </summary>
         public bool IsInTransaction { get; private set; }
 
-        public SqlConnection(string databasePath)
+        public static SqlConnection Create(IPersistentStorageFaultInjector faultInjector, string databasePath)
         {
-            var flags = OpenFlags.SQLITE_OPEN_CREATE | OpenFlags.SQLITE_OPEN_READWRITE;
+            faultInjector?.OnNewConnection();
 
+            var flags = OpenFlags.SQLITE_OPEN_CREATE | OpenFlags.SQLITE_OPEN_READWRITE;
             var result = (Result)raw.sqlite3_open_v2(databasePath, out var handle, (int)flags, vfs: null);
 
             if (result != Result.OK)
@@ -54,14 +61,23 @@ public SqlConnection(string databasePath)
 
             Contract.ThrowIfNull(handle);
 
+            raw.sqlite3_busy_timeout(handle, (int)TimeSpan.FromMinutes(1).TotalMilliseconds);
+
+            return new SqlConnection(faultInjector, handle);
+        }
+
+        private SqlConnection(IPersistentStorageFaultInjector faultInjector, sqlite3 handle)
+        {
+            _faultInjector = faultInjector;
             _handle = handle;
-            raw.sqlite3_busy_timeout(_handle, (int)TimeSpan.FromMinutes(1).TotalMilliseconds);
         }
 
         ~SqlConnection()
         {
             if (!Environment.HasShutdownStarted)
             {
+                var ex = new InvalidOperationException("SqlConnection was not properly closed");
+                _faultInjector?.OnFatalError(ex);
                 FatalError.Report(new InvalidOperationException("SqlConnection was not properly closed"));
             }
         }

src/Workspaces/Core/Desktop/Workspace/SQLite/SQLitePersistentStorage.cs

@@ -8,6 +8,7 @@
 using Microsoft.CodeAnalysis.Host;
 using Microsoft.CodeAnalysis.Options;
 using Microsoft.CodeAnalysis.SQLite.Interop;
+using Microsoft.CodeAnalysis.Storage;
 
 namespace Microsoft.CodeAnalysis.SQLite
 {
@@ -121,6 +122,8 @@ static SQLitePersistentStorage()
 
         private readonly CancellationTokenSource _shutdownTokenSource = new CancellationTokenSource();
 
+        private readonly IPersistentStorageFaultInjector _faultInjectorOpt;
+
         // Accessors that allow us to retrieve/store data into specific DB tables.  The
         // core Accessor type has logic that we to share across all reading/writing, while
         // the derived types contain only enough logic to specify how to read/write from
@@ -142,9 +145,11 @@ static SQLitePersistentStorage()
             string workingFolderPath,
             string solutionFilePath,
             string databaseFile,
-            Action<AbstractPersistentStorage> disposer)
+            Action<AbstractPersistentStorage> disposer,
+            IPersistentStorageFaultInjector faultInjectorOpt)
             : base(optionService, workingFolderPath, solutionFilePath, databaseFile, disposer)
         {
+            _faultInjectorOpt = faultInjectorOpt;
             _solutionAccessor = new SolutionAccessor(this);
             _projectAccessor = new ProjectAccessor(this);
             _documentAccessor = new DocumentAccessor(this);
@@ -162,7 +167,7 @@ private SqlConnection GetConnection()
             }
 
             // Otherwise create a new connection.
-            return new SqlConnection(this.DatabaseFile);
+            return SqlConnection.Create(_faultInjectorOpt, this.DatabaseFile);
         }
 
         private void ReleaseConnection(SqlConnection connection)
@@ -185,7 +190,15 @@ public override void Close()
         {
             // Flush all pending writes so that all data our features wanted written
             // are definitely persisted to the DB.
-            FlushAllPendingWritesAsync(CancellationToken.None).Wait();
+            try
+            {
+                FlushAllPendingWritesAsync(CancellationToken.None).Wait();
+            }
+            catch (Exception e)
+            {
+                // Flushing may fail.  We still have to close all our connections.
+                StorageDatabaseLogger.LogException(e);
+            }
 
             lock (_connectionGate)
             {

src/Workspaces/Core/Desktop/Workspace/SQLite/SQLitePersistentStorageService.cs

@@ -15,6 +15,8 @@ internal partial class SQLitePersistentStorageService : AbstractPersistentStorag
         private const string StorageExtension = "sqlite3";
         private const string PersistentStorageFileName = "storage.ide";
 
+        private readonly IPersistentStorageFaultInjector _faultInjectorOpt;
+
         public SQLitePersistentStorageService(
             IOptionService optionService,
             SolutionSizeTracker solutionSizeTracker)
@@ -22,9 +24,10 @@ internal partial class SQLitePersistentStorageService : AbstractPersistentStorag
         {
         }
 
-        public SQLitePersistentStorageService(IOptionService optionService, bool testing) 
-            : base(optionService, testing)
+        public SQLitePersistentStorageService(IOptionService optionService, IPersistentStorageFaultInjector faultInjector) 
+            : base(optionService, testing: true)
         {
+            _faultInjectorOpt = faultInjector;
         }
 
         protected override string GetDatabaseFilePath(string workingFolderPath)
@@ -35,7 +38,7 @@ protected override string GetDatabaseFilePath(string workingFolderPath)
 
         protected override AbstractPersistentStorage OpenDatabase(Solution solution, string workingFolderPath, string databaseFilePath)
             => new SQLitePersistentStorage(
-                OptionService, workingFolderPath, solution.FilePath, databaseFilePath, this.Release);
+                OptionService, workingFolderPath, solution.FilePath, databaseFilePath, this.Release, _faultInjectorOpt);
 
         protected override bool ShouldDeleteDatabase(Exception exception)
         {

src/Workspaces/Core/Desktop/Workspace/Storage/StorageDatabaseLogger.cs

@@ -11,12 +11,26 @@ internal class StorageDatabaseLogger
         private const string Kind = nameof(Kind);
         private const string Reason = nameof(Reason);
 
-        private static readonly ConcurrentDictionary<Type, object> s_set = new ConcurrentDictionary<Type, object>(concurrencyLevel: 2, capacity: 10);
+        private static readonly StorageDatabaseLogger Instance = new StorageDatabaseLogger();
+
+        private Exception _reportedException;
+        private string _reportedExceptionMessage;
+
+        private readonly ConcurrentDictionary<Type, Exception> _set = new ConcurrentDictionary<Type, Exception>(concurrencyLevel: 2, capacity: 10);
 
         internal static void LogException(Exception ex)
         {
+            Instance.LogExceptionWorker(ex);
+        }
+
+        private void LogExceptionWorker(Exception ex)
+        {
+            // hold onto last exception to make investigation easier
+            _reportedException = ex;
+            _reportedExceptionMessage = ex.ToString();
+
             // we already reported about this exception. also don't hold onto too many exceptions.
-            if (s_set.Count > 10 || !s_set.TryAdd(ex.GetType(), null))
+            if (_set.Count > 10 || !_set.TryAdd(ex.GetType(), ex))
             {
                 return;
             }

src/Workspaces/Core/Portable/Workspace/Host/PersistentStorage/IPersistentStorageFaultInjectionService.cs

+// 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;
+
+namespace Microsoft.CodeAnalysis.Host
+{
+    internal interface IPersistentStorageFaultInjector
+    {
+        void OnNewConnection();
+        void OnFatalError(Exception ex);
+    }
+}

src/Workspaces/Core/Portable/Workspaces.csproj

@@ -206,6 +206,7 @@
     </Compile>
     <Compile Include="ExtensionManager\IInfoBarService.cs" />
     <Compile Include="Remote\RemoteFeatureOptions.cs" />
+    <Compile Include="Workspace\Host\PersistentStorage\IPersistentStorageFaultInjectionService.cs" />
   </ItemGroup>
   <ItemGroup>
     <InternalsVisibleTo Include="csi" />