Don't Detach() a document already loaded for update (#6648)

OrchardCMS · Aug 6, 2020 · 3e5923c · 3e5923c
1 parent 0e1b331
commit 3e5923c
Show file tree

Hide file tree

Showing 16 changed files with 124 additions and 62 deletions.
diff --git a/src/OrchardCore.Modules/OrchardCore.AdminMenu/Services/AdminMenuService.cs b/src/OrchardCore.Modules/OrchardCore.AdminMenu/Services/AdminMenuService.cs
@@ -47,14 +47,19 @@ public async Task<AdminMenuList> GetAdminMenuListAsync()
             {
                 var changeToken = ChangeToken;
 
-                adminMenuList = await _sessionHelper.GetForCachingAsync<AdminMenuList>();
+                bool cacheable;
 
-                foreach (var adminMenu in adminMenuList.AdminMenu)
+                (cacheable, adminMenuList) = await _sessionHelper.GetForCachingAsync<AdminMenuList>();
+
+                if (cacheable)
                 {
-                    adminMenu.IsReadonly = true;
-                }
+                    foreach (var adminMenu in adminMenuList.AdminMenu)
+                    {
+                        adminMenu.IsReadonly = true;
+                    }
 
-                _memoryCache.Set(AdminMenuCacheKey, adminMenuList, changeToken);
+                    _memoryCache.Set(AdminMenuCacheKey, adminMenuList, changeToken);
+                }
             }
 
             return adminMenuList;

diff --git a/src/OrchardCore.Modules/OrchardCore.BackgroundTasks/Services/BackgroundTaskManager.cs b/src/OrchardCore.Modules/OrchardCore.BackgroundTasks/Services/BackgroundTaskManager.cs
@@ -47,14 +47,19 @@ public async Task<BackgroundTaskDocument> GetDocumentAsync()
             {
                 var changeToken = ChangeToken;
 
-                document = await _sessionHelper.GetForCachingAsync<BackgroundTaskDocument>();
+                bool cacheable;
 
-                foreach (var settings in document.Settings.Values)
+                (cacheable, document) = await _sessionHelper.GetForCachingAsync<BackgroundTaskDocument>();
+
+                if (cacheable)
                 {
-                    settings.IsReadonly = true;
-                }
+                    foreach (var settings in document.Settings.Values)
+                    {
+                        settings.IsReadonly = true;
+                    }
 
-                _memoryCache.Set(CacheKey, document, changeToken);
+                    _memoryCache.Set(CacheKey, document, changeToken);
+                }
             }
 
             return document;

diff --git a/src/OrchardCore.Modules/OrchardCore.Layers/Services/LayerService.cs b/src/OrchardCore.Modules/OrchardCore.Layers/Services/LayerService.cs
@@ -52,11 +52,16 @@ public async Task<LayersDocument> GetLayersAsync()
             {
                 var changeToken = ChangeToken;
 
-                layers = await _sessionHelper.GetForCachingAsync<LayersDocument>();
+                bool cacheable;
 
-                layers.IsReadonly = true;
+                (cacheable, layers) = await _sessionHelper.GetForCachingAsync<LayersDocument>();
 
-                _memoryCache.Set(LayersCacheKey, layers, changeToken);
+                if (cacheable)
+                {
+                    layers.IsReadonly = true;
+
+                    _memoryCache.Set(LayersCacheKey, layers, changeToken);
+                }
             }
 
             return layers;

diff --git a/src/OrchardCore.Modules/OrchardCore.Lucene/Services/LuceneIndexSettingsService.cs b/src/OrchardCore.Modules/OrchardCore.Lucene/Services/LuceneIndexSettingsService.cs
@@ -44,7 +44,12 @@ private async Task<LuceneIndexSettingsDocument> GetDocumentAsync()
             {
                 _changeToken = ChangeToken;
 
-                var document = await SessionHelper.GetForCachingAsync<LuceneIndexSettingsDocument>();
+                (var cacheable, var document) = await SessionHelper.GetForCachingAsync<LuceneIndexSettingsDocument>();
+
+                if (!cacheable)
+                {
+                    return document;
+                }
 
                 foreach (var name in document.LuceneIndexSettings.Keys)
                 {

diff --git a/src/OrchardCore.Modules/OrchardCore.Queries/Services/QueryManager.cs b/src/OrchardCore.Modules/OrchardCore.Queries/Services/QueryManager.cs
@@ -107,14 +107,19 @@ private async Task<QueriesDocument> GetDocumentAsync()
             {
                 var changeToken = ChangeToken;
 
-                queries = await _sessionHelper.GetForCachingAsync<QueriesDocument>();
+                bool cacheable;
 
-                foreach (var query in queries.Queries.Values)
+                (cacheable, queries) = await _sessionHelper.GetForCachingAsync<QueriesDocument>();
+
+                if (cacheable)
                 {
-                    query.IsReadonly = true;
-                }
+                    foreach (var query in queries.Queries.Values)
+                    {
+                        query.IsReadonly = true;
+                    }
 
-                _memoryCache.Set(QueriesDocumentCacheKey, queries, changeToken);
+                    _memoryCache.Set(QueriesDocumentCacheKey, queries, changeToken);
+                }
             }
 
             return queries;

diff --git a/src/OrchardCore.Modules/OrchardCore.Settings/Services/SiteService.cs b/src/OrchardCore.Modules/OrchardCore.Settings/Services/SiteService.cs
@@ -55,13 +55,18 @@ public async Task<ISite> GetSiteSettingsAsync()
                 // First get a new token.
                 var changeToken = ChangeToken;
 
+                bool cacheable;
+
                 // The cache is always updated with the actual persisted data.
-                site = await sessionHelper.GetForCachingAsync(GetDefaultSettings);
+                (cacheable, site) = await sessionHelper.GetForCachingAsync(GetDefaultSettings);
 
-                // Prevent data from being updated.
-                site.IsReadonly = true;
+                if (cacheable)
+                {
+                    // Prevent data from being updated.
+                    site.IsReadonly = true;
 
-                _memoryCache.Set(SiteCacheKey, site, changeToken);
+                    _memoryCache.Set(SiteCacheKey, site, changeToken);
+                }
             }
 
             return site;

diff --git a/src/OrchardCore.Modules/OrchardCore.Sitemaps/Services/SitemapManager.cs b/src/OrchardCore.Modules/OrchardCore.Sitemaps/Services/SitemapManager.cs
@@ -115,15 +115,19 @@ private async Task<SitemapDocument> GetDocumentAsync()
             {
                 var changeToken = ChangeToken;
 
-                document = await _sessionHelper.GetForCachingAsync<SitemapDocument>();
+                bool cacheable;
 
-                foreach (var sitemap in document.Sitemaps.Values)
-                {
-                    sitemap.IsReadonly = true;
-                }
+                (cacheable, document) = await _sessionHelper.GetForCachingAsync<SitemapDocument>();
 
-                _memoryCache.Set(SitemapsDocumentCacheKey, document, changeToken);
+                if (cacheable)
+                {
+                    foreach (var sitemap in document.Sitemaps.Values)
+                    {
+                        sitemap.IsReadonly = true;
+                    }
 
+                    _memoryCache.Set(SitemapsDocumentCacheKey, document, changeToken);
+                }
             }
 
             return document;

diff --git a/src/OrchardCore.Modules/OrchardCore.Templates/Services/AdminTemplatesManager.cs b/src/OrchardCore.Modules/OrchardCore.Templates/Services/AdminTemplatesManager.cs
@@ -46,14 +46,19 @@ public async Task<AdminTemplatesDocument> GetTemplatesDocumentAsync()
             {
                 var changeToken = ChangeToken;
 
-                document = await _sessionHelper.GetForCachingAsync<AdminTemplatesDocument>();
+                bool cacheable;
 
-                foreach (var template in document.Templates.Values)
+                (cacheable, document) = await _sessionHelper.GetForCachingAsync<AdminTemplatesDocument>();
+
+                if (cacheable)
                 {
-                    template.IsReadonly = true;
-                }
+                    foreach (var template in document.Templates.Values)
+                    {
+                        template.IsReadonly = true;
+                    }
 
-                _memoryCache.Set(CacheKey, document, changeToken);
+                    _memoryCache.Set(CacheKey, document, changeToken);
+                }
             }
 
             return document;

diff --git a/src/OrchardCore.Modules/OrchardCore.Templates/Services/TemplatesManager.cs b/src/OrchardCore.Modules/OrchardCore.Templates/Services/TemplatesManager.cs
@@ -46,14 +46,19 @@ public async Task<TemplatesDocument> GetTemplatesDocumentAsync()
             {
                 var changeToken = ChangeToken;
 
-                document = await _sessionHelper.GetForCachingAsync<TemplatesDocument>();
+                bool cacheable;
 
-                foreach (var template in document.Templates.Values)
+                (cacheable, document) = await _sessionHelper.GetForCachingAsync<TemplatesDocument>();
+
+                if (cacheable)
                 {
-                    template.IsReadonly = true;
-                }
+                    foreach (var template in document.Templates.Values)
+                    {
+                        template.IsReadonly = true;
+                    }
 
-                _memoryCache.Set(CacheKey, document, changeToken);
+                    _memoryCache.Set(CacheKey, document, changeToken);
+                }
             }
 
             return document;

diff --git a/src/OrchardCore/OrchardCore.ContentManagement.Abstractions/IContentDefinitionStore.cs b/src/OrchardCore/OrchardCore.ContentManagement.Abstractions/IContentDefinitionStore.cs
@@ -11,9 +11,10 @@ public interface IContentDefinitionStore
         Task<ContentDefinitionRecord> LoadContentDefinitionAsync();
 
         /// <summary>
-        /// Gets a single document (or create a new one) for caching and that should not be updated.
+        /// Gets a single document (or create a new one) for caching and that should not be updated,
+        /// and a bool indicating if it can be cached, not if it has been already loaded for update.
         /// </summary>
-        Task<ContentDefinitionRecord> GetContentDefinitionAsync();
+        Task<(bool, ContentDefinitionRecord)> GetContentDefinitionAsync();
 
         Task SaveContentDefinitionAsync(ContentDefinitionRecord contentDefinitionRecord);
     }

diff --git a/src/OrchardCore/OrchardCore.ContentManagement/ContentDefinitionManager.cs b/src/OrchardCore/OrchardCore.ContentManagement/ContentDefinitionManager.cs
@@ -332,9 +332,14 @@ private ContentDefinitionRecord GetContentDefinitionRecord()
                 },
                 state: null);
 
-                record = _contentDefinitionStore.GetContentDefinitionAsync().GetAwaiter().GetResult();
+                bool cacheable;
 
-                _memoryCache.Set(CacheKey, record, changeToken);
+                (cacheable, record) = _contentDefinitionStore.GetContentDefinitionAsync().GetAwaiter().GetResult();
+
+                if (cacheable)
+                {
+                    _memoryCache.Set(CacheKey, record, changeToken);
+                }
             }
 
             return record;

diff --git a/src/OrchardCore/OrchardCore.ContentManagement/DatabaseContentDefinitionStore.cs b/src/OrchardCore/OrchardCore.ContentManagement/DatabaseContentDefinitionStore.cs
@@ -25,7 +25,7 @@ public Task<ContentDefinitionRecord> LoadContentDefinitionAsync()
         /// <summary>
         /// Gets a single document (or create a new one) for caching and that should not be updated.
         /// </summary>
-        public Task<ContentDefinitionRecord> GetContentDefinitionAsync()
+        public Task<(bool, ContentDefinitionRecord)> GetContentDefinitionAsync()
             => _sessionHelper.GetForCachingAsync<ContentDefinitionRecord>();
 
         public Task SaveContentDefinitionAsync(ContentDefinitionRecord contentDefinitionRecord)

diff --git a/src/OrchardCore/OrchardCore.ContentManagement/FileContentDefinitionStore.cs b/src/OrchardCore/OrchardCore.ContentManagement/FileContentDefinitionStore.cs
@@ -33,14 +33,24 @@ public async Task<ContentDefinitionRecord> LoadContentDefinitionAsync()
                 return scopedCache.ContentDefinitionRecord;
             }
 
-            return scopedCache.ContentDefinitionRecord = await GetContentDefinitionAsync();
+            (_, var result) = await GetContentDefinitionAsync();
+
+            return scopedCache.ContentDefinitionRecord = result;
         }
 
         /// <summary>
         /// Gets a single document (or create a new one) for caching and that should not be updated.
         /// </summary>
-        public Task<ContentDefinitionRecord> GetContentDefinitionAsync()
+        public Task<(bool, ContentDefinitionRecord)> GetContentDefinitionAsync()
         {
+            var scopedCache = ShellScope.Services.GetRequiredService<FileContentDefinitionScopedCache>();
+
+            if (scopedCache.ContentDefinitionRecord != null)
+            {
+                // Return the already loaded document but indicating that it should not be cached.
+                return Task.FromResult((false, scopedCache.ContentDefinitionRecord));
+            }
+
             ContentDefinitionRecord result;
 
             if (!File.Exists(Filename))
@@ -59,7 +69,7 @@ public Task<ContentDefinitionRecord> GetContentDefinitionAsync()
                 }
             }
 
-            return Task.FromResult(result);
+            return Task.FromResult((true, result));
         }
 
         public Task SaveContentDefinitionAsync(ContentDefinitionRecord contentDefinitionRecord)

diff --git a/src/OrchardCore/OrchardCore.Data.Abstractions/ISessionHelper.cs b/src/OrchardCore/OrchardCore.Data.Abstractions/ISessionHelper.cs
@@ -12,17 +12,13 @@ public interface ISessionHelper
         /// Loads a single document (or create a new one) for updating and that should not be cached.
         /// For a full isolation, it needs to be used in pair with <see cref="GetForCachingAsync"/>.
         /// </summary>
-        /// <typeparam name="T"></typeparam>
-        /// <param name="factory">A factory methods to load or create a document.</param>
         Task<T> LoadForUpdateAsync<T>(Func<T> factory = null) where T : class, new();
 
         /// <summary>
-        /// Gets a single document (or create a new one) for caching and that should not be updated.
+        /// Gets a single document (or create a new one) for caching and that should not be updated,
+        /// and a bool indicating if it can be cached, not if it has been already loaded for update.
         /// For a full isolation, it needs to be used in pair with <see cref="LoadForUpdateAsync"/>.
         /// </summary>
-        /// <typeparam name="T"></typeparam>
-        /// <param name="factory">A factory method to get or create a document for caching.</param>
-        /// <returns></returns>
-        Task<T> GetForCachingAsync<T>(Func<T> factory = null) where T : class, new();
+        Task<(bool, T)> GetForCachingAsync<T>(Func<T> factory = null) where T : class, new();
     }
 }
diff --git a/src/OrchardCore/OrchardCore.Data/SessionHelper.cs b/src/OrchardCore/OrchardCore.Data/SessionHelper.cs
@@ -39,22 +39,23 @@ public SessionHelper(ISession session)
         }
 
         /// <inheritdocs/>
-        public async Task<T> GetForCachingAsync<T>(Func<T> factory = null) where T : class, new()
+        public async Task<(bool, T)> GetForCachingAsync<T>(Func<T> factory = null) where T : class, new()
         {
             if (_loaded.TryGetValue(typeof(T), out var loaded))
             {
-                _session.Detach(loaded);
+                // Return the already loaded document but indicating that it should not be cached.
+                return (false, loaded as T);
             }
 
             var document = await _session.Query<T>().FirstOrDefaultAsync();
 
             if (document != null)
             {
                 _session.Detach(document);
-                return document;
+                return (true, document);
             }
 
-            return factory?.Invoke() ?? new T();
+            return (true, factory?.Invoke() ?? new T());
         }
     }
 }
diff --git a/src/OrchardCore/OrchardCore.Infrastructure.Abstractions/Cache/IScopedDistributedCache.cs b/src/OrchardCore/OrchardCore.Infrastructure.Abstractions/Cache/IScopedDistributedCache.cs
@@ -24,21 +24,26 @@ public static Task SetAsync<T>(this IScopedDistributedCache scopedDistributedCac
             return scopedDistributedCache.SetAsync<T>(typeof(T).FullName, value);
         }
 
-        public static async Task<T> GetOrSetAsync<T>(this IScopedDistributedCache scopedDistributedCache, string key, Func<Task<T>> factory)
+        public static async Task<T> GetOrSetAsync<T>(this IScopedDistributedCache scopedDistributedCache, string key, Func<Task<(bool, T)>> factory)
         {
             var value = await scopedDistributedCache.GetAsync<T>(key);
 
             if (value == null)
             {
-                value = await factory();
+                bool cacheable;
 
-                await scopedDistributedCache.SetAsync(key, value);
+                (cacheable, value) = await factory();
+
+                if (cacheable)
+                {
+                    await scopedDistributedCache.SetAsync(key, value);
+                }
             }
 
             return value;
         }
 
-        public static Task<T> GetOrSetAsync<T>(this IScopedDistributedCache scopedDistributedCache, Func<Task<T>> factory)
+        public static Task<T> GetOrSetAsync<T>(this IScopedDistributedCache scopedDistributedCache, Func<Task<(bool, T)>> factory)
         {
             return scopedDistributedCache.GetOrSetAsync(typeof(T).FullName, factory);
         }