From 6db1e07b4788dc996b7815d1952ef92059c4cb68 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 18 Mar 2020 17:38:17 -0700 Subject: [PATCH] If we fail to create the sqlite db, don't continually retry for every client that wants it. --- .../AbstractPersistentStorageService.cs | 63 ++++++++----------- 1 file changed, 26 insertions(+), 37 deletions(-) diff --git a/src/Workspaces/Core/Portable/Storage/AbstractPersistentStorageService.cs b/src/Workspaces/Core/Portable/Storage/AbstractPersistentStorageService.cs index 1bb1a365ca2..a62c7fdeeb3 100644 --- a/src/Workspaces/Core/Portable/Storage/AbstractPersistentStorageService.cs +++ b/src/Workspaces/Core/Portable/Storage/AbstractPersistentStorageService.cs @@ -20,7 +20,6 @@ namespace Microsoft.CodeAnalysis.Storage /// internal abstract partial class AbstractPersistentStorageService : IChecksummedPersistentStorageService { - private readonly IOptionService _optionService; private readonly IPersistentStorageLocationService _locationService; /// @@ -30,13 +29,8 @@ internal abstract partial class AbstractPersistentStorageService : IChecksummedP private ReferenceCountedDisposable _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); @@ -68,7 +62,8 @@ internal IChecksummedPersistentStorage GetStorageWorker(Solution solution) // Do we already have storage for this? if (solution.Id == _currentPersistentStorageSolutionId) { - // We do, great + // We do, great up our ref count for our caller. They'll decrement it when done + // with it. return PersistentStorageReferenceCountedDisposableWrapper.AddReferenceCountToAndCreateWrapper(_currentPersistentStorage); } @@ -83,21 +78,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(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 +119,27 @@ private bool DatabaseSupported(Solution solution, bool checkBranchId) return true; } - private ReferenceCountedDisposable 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(persistentStorage); - } - - // okay, can't recover, then use no op persistent service - // so that things works old way (cache everything in memory) - return null; + return TryCreatePersistentStorage(solution, workingFolderPath) ?? + TryCreatePersistentStorage(solution, workingFolderPath) ?? + NoOpPersistentStorage.Instance; } - 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, out var persistentStorage) + ? persistentStorage + : null; } catch (Exception ex) { @@ -164,7 +153,7 @@ private ReferenceCountedDisposable TryCreatePersi IOUtilities.PerformIO(() => Directory.Delete(Path.GetDirectoryName(databaseFilePath), recursive: true)); } - return false; + return null; } } -- GitLab