Merge pull request #42551 from CyrusNajmabadi/sqliteFallbackToNoOp

If we fail to create the sqlite db, don't continually retry for every client that wants it.

src/Tools/IdeBenchmarks/SQLitePersistentStorageBenchmark.cs

@@ -55,8 +55,7 @@ public void GlobalSetup()
             _workspace.TryApplyChanges(_workspace.CurrentSolution.WithOptions(_workspace.Options
                 .WithChangedOption(StorageOptions.Database, StorageDatabase.SQLite)));
 
-            _storageService = new SQLitePersistentStorageService(
-                _workspace.Services.GetService<IOptionService>(), new LocationService());
+            _storageService = new SQLitePersistentStorageService(new LocationService());
 
             _storage = _storageService.GetStorageWorker(_workspace.CurrentSolution);
             if (_storage == NoOpPersistentStorage.Instance)

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

@@ -33,10 +33,6 @@ public enum Size
         private const string PersistentFolderPrefix = "PersistentStorageTests_";
 
         private readonly Encoding _encoding = Encoding.UTF8;
-        internal readonly IOptionService _persistentEnabledOptionService = new OptionServiceMock(new Dictionary<IOption, object>
-        {
-            { PersistentStorageOptions.Enabled, true },
-        });
 
         private AbstractPersistentStorageService _storageService;
         private readonly string _persistentFolder;

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

@@ -3,6 +3,8 @@
 // See the LICENSE file in the project root for more information.
 
 using System;
+using System.IO;
+using System.Threading.Tasks;
 using Microsoft.CodeAnalysis.Host;
 using Microsoft.CodeAnalysis.SQLite;
 using Microsoft.CodeAnalysis.Storage;
@@ -18,10 +20,10 @@ namespace Microsoft.CodeAnalysis.UnitTests.WorkspaceServices
     public class SQLitePersistentStorageTests : AbstractPersistentStorageTests
     {
         internal override AbstractPersistentStorageService GetStorageService(IPersistentStorageLocationService locationService, IPersistentStorageFaultInjector faultInjector)
-            => new SQLitePersistentStorageService(_persistentEnabledOptionService, locationService, faultInjector);
+            => new SQLitePersistentStorageService(locationService, faultInjector);
 
         [Fact]
-        public void TestCrashInNewConnection()
+        public async Task TestCrashInNewConnection()
         {
             var solution = CreateOrOpenSolution(nullPaths: true);
 
@@ -34,11 +36,22 @@ public void TestCrashInNewConnection()
                 },
                 onFatalError: e => throw e);
 
-            using (var storage = GetStorage(solution, faultInjector))
-            {
             // Because instantiating the connection will fail, we will not get back
             // a working persistent storage.
-                Assert.IsType<NoOpPersistentStorage>(storage);
+            using (var storage = GetStorage(solution, faultInjector))
+            using (var memStream = new MemoryStream())
+            using (var streamWriter = new StreamWriter(memStream))
+            {
+                streamWriter.WriteLine("contents");
+                streamWriter.Flush();
+
+                memStream.Position = 0;
+                await storage.WriteStreamAsync("temp", memStream);
+                var readStream = await storage.ReadStreamAsync("temp");
+
+                // Because we don't have a real storage service, we should get back
+                // null even when trying to read something we just wrote.
+                Assert.Null(readStream);
             }
 
             Assert.True(hitInjector);

src/Workspaces/Core/Portable/Storage/AbstractPersistentStorageService.cs

@@ -2,13 +2,14 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+#nullable enable
+
 using System;
 using System.IO;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.CodeAnalysis.ErrorReporting;
 using Microsoft.CodeAnalysis.Host;
-using Microsoft.CodeAnalysis.Options;
 using Microsoft.CodeAnalysis.Shared.Utilities;
 using Roslyn.Utilities;
 
@@ -20,26 +21,26 @@ namespace Microsoft.CodeAnalysis.Storage
     /// </summary>
     internal abstract partial class AbstractPersistentStorageService : IChecksummedPersistentStorageService
     {
-        private readonly IOptionService _optionService;
         private readonly IPersistentStorageLocationService _locationService;
 
         /// <summary>
         /// This lock guards all mutable fields in this type.
         /// </summary>
         private readonly object _lock = new object();
-        private ReferenceCountedDisposable<IChecksummedPersistentStorage> _currentPersistentStorage;
-        private SolutionId _currentPersistentStorageSolutionId;
+        private ReferenceCountedDisposable<IChecksummedPersistentStorage>? _currentPersistentStorage;
+        private SolutionId? _currentPersistentStorageSolutionId;
 
-        protected AbstractPersistentStorageService(
-            IOptionService optionService,
-            IPersistentStorageLocationService locationService)
-        {
-            _optionService = optionService;
-            _locationService = locationService;
-        }
+        protected AbstractPersistentStorageService(IPersistentStorageLocationService locationService)
+            => _locationService = locationService;
 
         protected abstract string GetDatabaseFilePath(string workingFolderPath);
-        protected abstract bool TryOpenDatabase(Solution solution, string workingFolderPath, string databaseFilePath, out IChecksummedPersistentStorage storage);
+
+        /// <summary>
+        /// Can throw.  If it does, the caller (<see cref="CreatePersistentStorage"/>) will attempt
+        /// to delete the database and retry opening one more time.  If that fails again, the <see
+        /// cref="NoOpPersistentStorage"/> instance will be used.
+        /// </summary>
+        protected abstract IChecksummedPersistentStorage? TryOpenDatabase(Solution solution, string workingFolderPath, string databaseFilePath);
         protected abstract bool ShouldDeleteDatabase(Exception exception);
 
         IPersistentStorage IPersistentStorageService.GetStorage(Solution solution)
@@ -68,8 +69,9 @@ internal IChecksummedPersistentStorage GetStorageWorker(Solution solution)
                 // Do we already have storage for this?
                 if (solution.Id == _currentPersistentStorageSolutionId)
                 {
-                    // We do, great
-                    return PersistentStorageReferenceCountedDisposableWrapper.AddReferenceCountToAndCreateWrapper(_currentPersistentStorage);
+                    // We do, great. Increment our ref count for our caller.  They'll decrement it
+                    // when done with it.
+                    return PersistentStorageReferenceCountedDisposableWrapper.AddReferenceCountToAndCreateWrapper(_currentPersistentStorage!);
                 }
 
                 var workingFolder = _locationService.TryGetStorageLocation(solution);
@@ -83,21 +85,27 @@ internal IChecksummedPersistentStorage GetStorageWorker(Solution solution)
                 {
                     var storageToDispose = _currentPersistentStorage;
 
+                    // Kick off a task to actually go dispose the previous cached storage instance.
+                    // This will remove the single ref count we ourselves added when we cached the
+                    // instance.  Then once all other existing clients who are holding onto this
+                    // instance let go, it will finally get truly disposed.
                     Task.Run(() => storageToDispose.Dispose());
 
                     _currentPersistentStorage = null;
                     _currentPersistentStorageSolutionId = null;
                 }
 
-                _currentPersistentStorage = TryCreatePersistentStorage(solution, workingFolder);
-
-                if (_currentPersistentStorage == null)
-                {
-                    return NoOpPersistentStorage.Instance;
-                }
+                var storage = CreatePersistentStorage(solution, workingFolder);
+                Contract.ThrowIfNull(storage);
 
+                // Create and cache a new storage instance associated with this particular solution.
+                // It will initially have a ref-count of 1 due to our reference to it.
+                _currentPersistentStorage = new ReferenceCountedDisposable<IChecksummedPersistentStorage>(storage);
                 _currentPersistentStorageSolutionId = solution.Id;
 
+                // Now increment the reference count and return to our caller.  The current ref
+                // count for this instance will be 2.  Until all the callers *and* us decrement
+                // the refcounts, this instance will not be actually disposed.
                 return PersistentStorageReferenceCountedDisposableWrapper.AddReferenceCountToAndCreateWrapper(_currentPersistentStorage);
             }
         }
@@ -118,39 +126,25 @@ private bool DatabaseSupported(Solution solution, bool checkBranchId)
             return true;
         }
 
-        private ReferenceCountedDisposable<IChecksummedPersistentStorage> TryCreatePersistentStorage(Solution solution, string workingFolderPath)
+        private IChecksummedPersistentStorage CreatePersistentStorage(Solution solution, string workingFolderPath)
         {
             // Attempt to create the database up to two times.  The first time we may encounter
             // some sort of issue (like DB corruption).  We'll then try to delete the DB and can
             // try to create it again.  If we can't create it the second time, then there's nothing
             // we can do and we have to store things in memory.
-            if (TryCreatePersistentStorage(solution, workingFolderPath, out var persistentStorage) ||
-                TryCreatePersistentStorage(solution, workingFolderPath, out persistentStorage))
-            {
-                return new ReferenceCountedDisposable<IChecksummedPersistentStorage>(persistentStorage);
+            return TryCreatePersistentStorage(solution, workingFolderPath) ??
+                   TryCreatePersistentStorage(solution, workingFolderPath) ??
+                   NoOpPersistentStorage.Instance;
         }
 
-            // okay, can't recover, then use no op persistent service 
-            // so that things works old way (cache everything in memory)
-            return null;
-        }
-
-        private bool TryCreatePersistentStorage(
+        private IChecksummedPersistentStorage? TryCreatePersistentStorage(
             Solution solution,
-            string workingFolderPath,
-            out IChecksummedPersistentStorage persistentStorage)
+            string workingFolderPath)
         {
-            persistentStorage = null;
-
             var databaseFilePath = GetDatabaseFilePath(workingFolderPath);
             try
             {
-                if (!TryOpenDatabase(solution, workingFolderPath, databaseFilePath, out persistentStorage))
-                {
-                    return false;
-                }
-
-                return true;
+                return TryOpenDatabase(solution, workingFolderPath, databaseFilePath);
             }
             catch (Exception ex)
             {
@@ -164,13 +158,13 @@ private ReferenceCountedDisposable<IChecksummedPersistentStorage> TryCreatePersi
                     IOUtilities.PerformIO(() => Directory.Delete(Path.GetDirectoryName(databaseFilePath), recursive: true));
                 }
 
-                return false;
+                return null;
             }
         }
 
         private void Shutdown()
         {
-            ReferenceCountedDisposable<IChecksummedPersistentStorage> storage = null;
+            ReferenceCountedDisposable<IChecksummedPersistentStorage>? storage = null;
 
             lock (_lock)
             {
@@ -217,7 +211,9 @@ private PersistentStorageReferenceCountedDisposableWrapper(ReferenceCountedDispo
 
             public static IChecksummedPersistentStorage AddReferenceCountToAndCreateWrapper(ReferenceCountedDisposable<IChecksummedPersistentStorage> storage)
             {
-                return new PersistentStorageReferenceCountedDisposableWrapper(storage.TryAddReference());
+                // This should only be called from a caller that has a non-null storage that it
+                // already has a reference on.  So .TryAddReference cannot fail.
+                return new PersistentStorageReferenceCountedDisposableWrapper(storage.TryAddReference() ?? throw ExceptionUtilities.Unreachable);
             }
 
             public void Dispose()

src/Workspaces/Core/Portable/Storage/PersistenceStorageServiceFactory.cs

@@ -32,9 +32,7 @@ public IWorkspaceService CreateService(HostWorkspaceServices workspaceServices)
                 case StorageDatabase.SQLite:
                     var locationService = workspaceServices.GetService<IPersistentStorageLocationService>();
                     if (locationService != null)
-                    {
-                        return new SQLitePersistentStorageService(optionService, locationService);
-                    }
+                        return new SQLitePersistentStorageService(locationService);
 
                     break;
             }

src/Workspaces/Core/Portable/Storage/SQLite/SQLitePersistentStorageService.cs

@@ -2,11 +2,12 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+#nullable enable
+
 using System;
 using System.IO;
 using System.Runtime.InteropServices;
 using Microsoft.CodeAnalysis.Host;
-using Microsoft.CodeAnalysis.Options;
 using Microsoft.CodeAnalysis.Shared.Utilities;
 using Microsoft.CodeAnalysis.Storage;
 using Roslyn.Utilities;
@@ -19,7 +20,7 @@ internal partial class SQLitePersistentStorageService : AbstractPersistentStorag
         private const string StorageExtension = "sqlite3";
         private const string PersistentStorageFileName = "storage.ide";
 
-        private readonly IPersistentStorageFaultInjector _faultInjectorOpt;
+        private readonly IPersistentStorageFaultInjector? _faultInjectorOpt;
 
         [DllImport("kernel32.dll")]
         private static extern IntPtr LoadLibrary(string dllToLoad);
@@ -41,6 +42,8 @@ private static bool TryInitializeLibrariesLazy()
             {
                 var myFolder = Path.GetDirectoryName(
                     typeof(SQLitePersistentStorage).Assembly.Location);
+                if (myFolder == null)
+                    return false;
 
                 var is64 = IntPtr.Size == 8;
                 var subfolder = is64 ? "x64" : "x86";
@@ -62,18 +65,15 @@ private static bool TryInitializeLibrariesLazy()
             return true;
         }
 
-        public SQLitePersistentStorageService(
-            IOptionService optionService,
-            IPersistentStorageLocationService locationService)
-            : base(optionService, locationService)
+        public SQLitePersistentStorageService(IPersistentStorageLocationService locationService)
+            : base(locationService)
         {
         }
 
         public SQLitePersistentStorageService(
-            IOptionService optionService,
             IPersistentStorageLocationService locationService,
             IPersistentStorageFaultInjector faultInjector)
-            : this(optionService, locationService)
+            : this(locationService)
         {
             _faultInjectorOpt = faultInjector;
         }
@@ -84,25 +84,23 @@ protected override string GetDatabaseFilePath(string workingFolderPath)
             return Path.Combine(workingFolderPath, StorageExtension, PersistentStorageFileName);
         }
 
-        protected override bool TryOpenDatabase(
-            Solution solution, string workingFolderPath, string databaseFilePath, out IChecksummedPersistentStorage storage)
+        protected override IChecksummedPersistentStorage? TryOpenDatabase(
+            Solution solution, string workingFolderPath, string databaseFilePath)
         {
             if (!TryInitializeLibraries())
             {
                 // SQLite is not supported on the current platform
-                storage = null;
-                return false;
+                return null;
             }
 
             // try to get db ownership lock. if someone else already has the lock. it will throw
             var dbOwnershipLock = TryGetDatabaseOwnership(databaseFilePath);
             if (dbOwnershipLock == null)
             {
-                storage = null;
-                return false;
+                return null;
             }
 
-            SQLitePersistentStorage sqlStorage = null;
+            SQLitePersistentStorage? sqlStorage = null;
             try
             {
                 sqlStorage = new SQLitePersistentStorage(
@@ -110,6 +108,7 @@ protected override string GetDatabaseFilePath(string workingFolderPath)
 
                 sqlStorage.Initialize(solution);
 
+                return sqlStorage;
             }
             catch (Exception)
             {
@@ -126,22 +125,26 @@ protected override string GetDatabaseFilePath(string workingFolderPath)
                 }
                 throw;
             }
-
-            storage = sqlStorage;
-            return true;
         }
 
-        private static IDisposable TryGetDatabaseOwnership(string databaseFilePath)
+        /// <summary>
+        /// Returns null in the case where an IO exception prevented us from being able to acquire
+        /// the db lock file.
+        /// </summary>
+        private static IDisposable? TryGetDatabaseOwnership(string databaseFilePath)
         {
-            return IOUtilities.PerformIO<IDisposable>(() =>
+            return IOUtilities.PerformIO<IDisposable?>(() =>
             {
                 // make sure directory exist first.
                 EnsureDirectory(databaseFilePath);
 
+                var directoryName = Path.GetDirectoryName(databaseFilePath);
+                Contract.ThrowIfNull(directoryName);
+
                 return File.Open(
-                    Path.Combine(Path.GetDirectoryName(databaseFilePath), LockFile),
+                    Path.Combine(directoryName, LockFile),
                     FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None);
-            });
+            }, defaultValue: null);
         }
 
         private static void EnsureDirectory(string databaseFilePath)