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

Jump to bottom

SimpleCache should use ConcurrentDictionary #583

Closed
zakwillis opened this issue Apr 10, 2019 · 10 comments
Closed

SimpleCache should use ConcurrentDictionary #583

zakwillis opened this issue Apr 10, 2019 · 10 comments
Assignees
@tidyui
Labels
core enhancement question
Milestone
Version 6.1

Comments

@zakwillis
Copy link

Hello. I put the following site online. http://www.inforhino.co.uk/

It returned the above error. Pasting the details in at the end of the email. To resolve this, I recycled the app pool and the site is up again.

The site is live, but I still consider my site to be in beta phase, as I have lots of cosmetic and content fixes to make. My idea is to use Piranha CMS for my next phase of my property platform I am launching. Ultimately, I am working on some quite interesting features I may eventually push back into the piranha project or simply tell you guys about it.

I am trying to avoid digging into the actual Piranha CMS source as I am certain you guys are still working on it. I can add an exception page/forwarder/handler but recreating exception may prove a challenge?

I myself, try to avoid going down this path - but it may be necessary? https://docs.microsoft.com/en-us/dotnet/api/system.collections.concurrent.concurrentdictionary-2?view=netframework-4.7.2

The page cannot be displayed because an internal server error has occurred.nt; font-size: 1.2em; text-align: left; text-decoration: none; display: inline-block; border: 0; padding: 0; } .rawExceptionStackTrace { font-size: 1.2em; } .rawExceptionBlock { border-top: 1px #ddd solid; border-bottom: 1px #ddd solid; } .showRawExceptionContainer { margin-top: 10px; margin-bottom: 10px; } .expandCollapseButton { cursor: pointer; float: left; height: 16px; width: 16px; font-size: 10px; position: absolute; left: 10px; background-color: #eee; padding: 0; border: 0; margin: 0; }
An unhandled exception occurred while processing the request.
InvalidOperationException: Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.
System.Collections.Generic.Dictionary<TKey, TValue>.FindEntry(TKey key)
Stack
Query
Cookies
Headers
InvalidOperationException: Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.
System.Collections.Generic.Dictionary<TKey, TValue>.FindEntry(TKey key)
System.Collections.Generic.Dictionary<TKey, TValue>.TryGetValue(TKey key, out TValue value)
Piranha.Cache.MemCache.Get(string key)
Piranha.Repositories.SiteRepository.GetByHostname(string hostname)
Piranha.AspNetCore.Services.ApplicationService.Init(HttpContext context)
Piranha.AspNetCore.ApplicationMiddleware.Invoke(HttpContext context, IApi api, IApplicationService service)
Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
Microsoft.AspNetCore.StaticFiles.StaticFileMiddleware.Invoke(HttpContext context)
Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

Show raw exception details
System.InvalidOperationException: Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.

at System.Collections.Generic.Dictionary`2.FindEntry(TKey key)

at System.Collections.Generic.Dictionary`2.TryGetValue(TKey key, TValue& value)

at Piranha.Cache.MemCache.Get[T](String key)

at Piranha.Repositories.SiteRepository.GetByHostname(String hostname)

at Piranha.AspNetCore.Services.ApplicationService.Init(HttpContext context)

at Piranha.AspNetCore.ApplicationMiddleware.Invoke(HttpContext context, IApi api, IApplicationService service)

at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)

at Microsoft.AspNetCore.StaticFiles.StaticFileMiddleware.Invoke(HttpContext context)

at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)
@zakwillis
Copy link
Author

Hello.
I think the fix is to do the following.
private readonly IDictionary<string, object> _cache = new ConcurrentDictionary<string, object>(); // new Dictionary<string, object>();

My rationale is based upon the below...

   private readonly IDictionary<string, object> _cache = new Dictionary<string, object>();

would need to be something like...
private ConcurrentDictionary<string,Object> newcache = new ConcurrentDictionary<string, object>();

We see that ConcurrentDictionary implements IDictionary so it should be okay to change this?

public class ConcurrentDictionary<TKey, TValue> : ICollection<KeyValuePair<TKey, TValue>>, IEnumerable<KeyValuePair<TKey, TValue>>, IEnumerable, IDictionary<TKey, TValue>, IReadOnlyCollection<KeyValuePair<TKey, TValue>>, IReadOnlyDictionary<TKey, TValue>, ICollection, IDictionary

@stefanolsen
Copy link
Contributor

I think you are using the wrong cache implementation. MemCache or SimpleCache is not thread-safe.
Using MemoryCache instead (documented here) will fix this quickly.
However, using ConcurrentDictionary would be the right fix for the SimpleCache in the long run.

@zakwillis
Copy link
Author

zakwillis commented Apr 11, 2019 •
edited
Loading

Hello Stefan. So, I read the documentation. In my startup.cs I do indeed use Memcache
services.AddPiranhaMemCache();
BUT I also need to do this...
services.AddTransient<ICache, MemCache>();

Will give it a whirl.

I guess when you get the opportunity, shove the ConcurrentDictionary in there.

Many thanks,

Zak

@tidyui
Copy link
Member

tidyui commented Apr 13, 2019 •
edited
Loading

Hi there! The MemCache is an older class for an older version which has been renamed with SimpleCache. In 6.0 a DeepClone was added when an item was returned from cache, in earlier versions, all threads requesting the same cached object got a reference to the same instance and could possibly corrupt it. Which version of Piranha are you using?

@zakwillis
Copy link
Author

zakwillis commented Apr 13, 2019 via email

@zakwillis
Copy link
Author

zakwillis commented Apr 13, 2019 via email

@zakwillis
Copy link
Author

Hello there. Forgive me if I have made any oversights or if this is quite involved.
I think the crux of the issue is. If I follow the guidance to install a new template it is referring to older package versions. http://piranhacms.org/docs/introduction/getting-started/project-templates results in versions 5.3.0

The critical question is. Should it be possible to run an upgrade of the project template you provide to your later version of piranha CMS to permit correct use of the caching service?

Background
I followed the guidance from here http://piranhacms.org/docs/introduction/getting-started/project-templates .
Taking that template, I focused more on adding a couple of page types and changing the CSS/Layout/Menu to match my requirements rather than reinventing the wheel.

I just copied my project to a new folder, ran an update of all Piranha Nuget Packages. Puts most Piranha at versions 6 or above. This now introduces a number of async call issues which I have fixed on my local version.

Current status

There are two main errors as I see it;
Startup.cs
services.AddPiranhaEF( options =>
options.UseSqlServer(connString));
Documented here. http://piranhacms.org/docs/architecture/databases/sqlserver

CMS Controller Archive method
I started editing this but it seems the models are inconsistent now.

Raw errors from Visual Studio
Severity Code Description Project File Line Suppression State
Error CS0619 'ArchiveService.GetByTagId(Guid, Guid, int?, int?, int?, int?)' is obsolete: 'The archive service now only loads the PostArchive and not the page. Please refer to GetByIdAsync()' IRWebsiteCMS C:\InfoRhino\Test\IRWebsite\IRWebsiteCMS\Controllers\CmsController.cs 39 Active
Error CS0619 'ArchiveService.GetById(Guid, int?, int?, int?, int?)' is obsolete: 'The archive service now only loads the PostArchive and not the page. Please refer to GetByIdAsync()' IRWebsiteCMS C:\InfoRhino\Test\IRWebsite\IRWebsiteCMS\Controllers\CmsController.cs 40 Active
Error CS1061 'IServiceCollection' does not contain a definition for 'AddPiranhaEF' and no accessible extension method 'AddPiranhaEF' accepting a first argument of type 'IServiceCollection' could be found (are you missing a using directive or an assembly reference?) IRWebsiteCMS C:\InfoRhino\Test\IRWebsite\IRWebsiteCMS\Startup.cs 183 Active

[Route("archive")]
public async Task Archive(Guid id, int? year = null, int? month = null, int? page = null,
Guid? category = null, Guid? tag = null)
{
Models.BlogArchive model;

        if (category.HasValue)
            model = await api.Archives.GetByIdAsync<Models.BlogArchive>(archiveId: id, categoryId: category.Value, currentPage: page, year: year, month: month);
        else if (tag.HasValue)

I haven't continued trying to fix these two calls...
model = api.Archives.GetByTagId<Models.BlogArchive>(id, tag.Value, page, year, month);
else model = api.Archives.GetById<Models.BlogArchive>(id, page, year, month);

        return View(model);
    }

@zakwillis
Copy link
Author

Hi guys. Did what I add make sense? Quite a bit of information - I gave.
The main observation is, the sample project your documentation says to use on your website is not updated to run with the latest version of Piranha CMS Core.
I believe based upon @stefanolsen 's observation. I can keep my version of Piranha running fine by implementing the correct cache.

So the only outcome is to ensure the sample project is running on your latest version. This is something I could look at once I get past my current phase of my project which is a few month's off.
Similarly, I don't mind putting your DB into SQL Server Project and having a separate DB project.

Many thanks,

Zak

@tidyui
Copy link
Member

tidyui commented Apr 24, 2019

To answer your previous question, no you can't update an already created project with a project template. Project templates just create the baseline for a new project. Like you noticed the documentation was wrong on the website in pointing to a specific version of the project templates. We've updated this and we will also update SimpleCache to use the ConcurrentDictionary for version 6.1

@tidyui tidyui added this to the Version 6.1 milestone Apr 24, 2019
@tidyui tidyui closed this as completed in 02f8130 Apr 24, 2019
@zakwillis
Copy link
Author

Hi, I just ran the templates again, downloaded and run. So, at some point I will port everything over to the newer version. Many thanks, Zak

@tidyui tidyui self-assigned this Apr 25, 2019
@tidyui tidyui changed the title InvalidOperationException: Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct. SimpleCache should use ConcurrentDictionary May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tidyui tidyui

Labels
core enhancement question
Projects
None yet
Milestone
Version 6.1
Development

No branches or pull requests
3 participants
@tidyui @stefanolsen @zakwillis