Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Aug 6, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
534f675
Fixes #6356 Content step in a migration recipe fails
jtkech Jul 13, 2020
a7e8ed2
Minor changes
jtkech Jul 14, 2020
c27ac3a
Auto swap from Get for caching to Load for update
jtkech Jul 16, 2020
888a230
Comments
jtkech Jul 18, 2020
bbb962e
Merge remote-tracking branch 'origin/dev' into jtkech/recipe-migration
jtkech Jul 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions src/OrchardCore.Modules/OrchardCore.AdminMenu/Services/AdminMenuService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
15 changes: 10 additions & 5 deletions src/OrchardCore.Modules/OrchardCore.BackgroundTasks/Services/BackgroundTaskManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
11 changes: 8 additions & 3 deletions src/OrchardCore.Modules/OrchardCore.Layers/Services/LayerService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
7 changes: 6 additions & 1 deletion src/OrchardCore.Modules/OrchardCore.Lucene/Services/LuceneIndexSettingsService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
15 changes: 10 additions & 5 deletions src/OrchardCore.Modules/OrchardCore.Queries/Services/QueryManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
13 changes: 9 additions & 4 deletions src/OrchardCore.Modules/OrchardCore.Settings/Services/SiteService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
16 changes: 10 additions & 6 deletions src/OrchardCore.Modules/OrchardCore.Sitemaps/Services/SitemapManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
15 changes: 10 additions & 5 deletions src/OrchardCore.Modules/OrchardCore.Templates/Services/AdminTemplatesManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
15 changes: 10 additions & 5 deletions src/OrchardCore.Modules/OrchardCore.Templates/Services/TemplatesManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
5 changes: 3 additions & 2 deletions src/OrchardCore/OrchardCore.ContentManagement.Abstractions/IContentDefinitionStore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
9 changes: 7 additions & 2 deletions src/OrchardCore/OrchardCore.ContentManagement/ContentDefinitionManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/OrchardCore/OrchardCore.ContentManagement/DatabaseContentDefinitionStore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the "caching" information should not leak at this level. It should always be either cacheable or not. Another solution is to change this method to two:

  • CreateContentDefinitionRecord
  • GetContentDefinitionRecord

And the user would check if Get returned null before calling Create. Right now it's more like a GetOrCreate() and the cachability of the result will vary.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, i'm open to any suggestions

That said, this is no more the case in the distributedCache branch, it is done at the IDocumentStore level and only checked at the IDocumentManager<> level, then any service using an IDocumentManager<> don't have to care about this.

/// <inheritdoc />
public Task<ContentDefinitionRecord> LoadContentDefinitionAsync() => _documentManager.GetMutableAsync();

/// <inheritdoc />
public Task<ContentDefinitionRecord> GetContentDefinitionAsync() => _documentManager.GetImmutableAsync();

It should always be either cacheable or not

Yes i agree but this is the singularity i described in my comments. In some rare cases the caller should not create a new instance but should not cache the returned one. But this only happens in a given scope where we can't refresh the cache for a document that has already been loaded for update and that may have been flushed.

it's more like a GetOrCreate() and the cachability of the result will vary.

Yes exactly, before it was already a GetOrCreate() with a factory, but, because of the above, even the result is not null it doesn't mean that it can be cached

I will think about it

=> _sessionHelper.GetForCachingAsync<ContentDefinitionRecord>();

public Task SaveContentDefinitionAsync(ContentDefinitionRecord contentDefinitionRecord)
Expand Down
16 changes: 13 additions & 3 deletions src/OrchardCore/OrchardCore.ContentManagement/FileContentDefinitionStore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -59,7 +69,7 @@ public Task<ContentDefinitionRecord> GetContentDefinitionAsync()
}
}

return Task.FromResult(result);
return Task.FromResult((true, result));
}

public Task SaveContentDefinitionAsync(ContentDefinitionRecord contentDefinitionRecord)
Expand Down
10 changes: 3 additions & 7 deletions src/OrchardCore/OrchardCore.Data.Abstractions/ISessionHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
9 changes: 5 additions & 4 deletions src/OrchardCore/OrchardCore.Data/SessionHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More like GetOrCreate than Get. That would make the factory usage obvious.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here, in the distributedCache branch, it is done at the IDocumentStore (can be an IFileDocumentStore) level, same problem but cacheable is only checked at the IDocumentManager level.

More like GetOrCreate than Get. That would make the factory usage obvious.

Yes i agree, before the change it was already using a factory, would you mean GetOrCreateForCachingAsync? In the distributedCache branch the related method is GetImmutableAsync() that would become GetOrCreateImmutableAsync()

I will think about it, but again i'm open to any suggestions, let me know

{
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());
}
}
}
13 changes: 9 additions & 4 deletions src/OrchardCore/OrchardCore.Infrastructure.Abstractions/Cache/IScopedDistributedCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down