nopCommerce 4.30 - Bug fixes and improvements

Nop-Templates.com wrote:
Thanks, Boyko! We'll check it soon - https://github.com/nopSolutions/nopCommerce/issues/4434

Hi guys,

In addition to the above I think there is an issue with the ACL in general.
ACL changes do not seem to take effect immediately but require a restart of the site.
Here is an example to try:

1. Disable the Shopping Cart for Guest role from the ACL page and Save it.
2. Open the site in an incognito window (as a Guest) and you can still see the Shopping Cart.

Thanks,
Boyko

Hi guys,

In addition to the above I think there is an issue with the ACL in general.
ACL changes do not seem to take effect immediately but require a restart of the site.
Here is an example to try:

1. Disable the Shopping Cart for Guest role from the ACL page and Save it.
2. Open the site in an incognito window (as a Guest) and you can still see the Shopping Cart.

Thanks,
Boyko

Hi nopCommerce,

I saw mention of big performance upgrades, have improvements to the performance of redis ( see issue https://github.com/nopSolutions/nopCommerce/issues/3875 & https://github.com/nopSolutions/nopCommerce/pull/4255) scheduled for the nopCommerce 4.30 release ?

Hi Boyko! We fixed both ACL page problem. Please see this commit for mo details

Firstly, many congratulation nopCommerce team for 4.30

The highlighted feature was quite impressive such as Upgrade to .NET Core 3.1, MySQL support, Facebook Pixel changes, performance enhancement and lot's of bug fixes.

Since 2 day i was underlying with nopCommerce 4.30 beta testing and i found some of the bugs too, here i listed all the bugs.

Majority of bug fixed by nopCommerce team member and the way team fixed with short time was very majestic.

I urge every active community member take participate on nopCommerce 4.30 beta testing.


1)  https://github.com/nopSolutions/nopCommerce/issues/4385
2)  https://github.com/nopSolutions/nopCommerce/issues/4386
3)  https://github.com/nopSolutions/nopCommerce/issues/4387
4)  https://github.com/nopSolutions/nopCommerce/issues/4388
5)  https://github.com/nopSolutions/nopCommerce/issues/4390
6)  https://github.com/nopSolutions/nopCommerce/issues/4391
7)  https://github.com/nopSolutions/nopCommerce/issues/4392
8)  https://github.com/nopSolutions/nopCommerce/issues/4393
9)  https://github.com/nopSolutions/nopCommerce/issues/4395
10) https://github.com/nopSolutions/nopCommerce/issues/4397
11) https://github.com/nopSolutions/nopCommerce/issues/4399
12) https://github.com/nopSolutions/nopCommerce/issues/4409
13) https://github.com/nopSolutions/nopCommerce/issues/4419
14) https://github.com/nopSolutions/nopCommerce/issues/4433
15) https://github.com/nopSolutions/nopCommerce/issues/4389
16) https://github.com/nopSolutions/nopCommerce/issues/4417
17) https://github.com/nopSolutions/nopCommerce/issues/4443
18) https://github.com/nopSolutions/nopCommerce/issues/4445
19) https://github.com/nopSolutions/nopCommerce/issues/4446
20) https://github.com/nopSolutions/nopCommerce/issues/4449
21) https://github.com/nopSolutions/nopCommerce/issues/4450
22) https://github.com/nopSolutions/nopCommerce/issues/4451
23) https://github.com/nopSolutions/nopCommerce/issues/4453

Sergei-k wrote:

Hi Sergei,

Thank you! I can confirm the both issues are fixed now.

Thanks,
Boyko

Hi guys,

I did some load tests i.e 1800 requests in 60 seconds and managed to consistently reproduce an error in the caching (only 2 or 3 of all requests fail with that error).
It is basically an error caused when several threads/requests access the CacheKey objects that are static and shared between all threads and eventually you end up with the error below:

Collection was modified; enumeration operation may not execute.

at System.ThrowHelper.ThrowInvalidOperationException_InvalidOperation_EnumFailedVersion()
   at System.Collections.Generic.List`1.Enumerator.MoveNextRare()
   at Nop.Core.Caching.MemoryCacheManager.SetupEntry(ICacheEntry entry, CacheKey key) in C:\DEV\GitHub\nopCommerce_latest\src\Libraries\Nop.Core\Caching\MemoryCacheManager.cs:line 75
   at Nop.Core.Caching.MemoryCacheManager.<>c__DisplayClass7_0`1.<Get>b__0(ICacheEntry entry) in C:\DEV\GitHub\nopCommerce_latest\src\Libraries\Nop.Core\Caching\MemoryCacheManager.cs:line 58
   at Microsoft.Extensions.Caching.Memory.CacheExtensions.GetOrCreate[TItem](IMemoryCache cache, Object key, Func`2 factory)

You won't be able to reproduce it easily unless you make a number of simultaneous requests but the problem is a quite standard one so you don't really need to reproduce it in order to fix it.
It seems like the foreach  of the key.Prefixes in the SetupEntry method in MemoryCacheManager is the problematic one since it uses enumerator and when the elements of the Prefixes collection are changed at the same time you enumerate the collection you will end up with this error.
Here is the exact line in the code:
https://github.com/nopSolutions/nopCommerce/blob/8c1e1c59aea1a5cbb8fd15d335773d2c2508fb73/src/Libraries/Nop.Core/Caching/MemoryCacheManager.cs#L69

I changed the code to simply use a for loop rather than a foreach statement and this seems to fixes the problem. Here is the code that I used:

for (var i = 0; i < key.Prefixes.Count; i++)
            {
                var tokenSource = _prefixes.GetOrAdd(key.Prefixes[i], new CancellationTokenSource());

                entry.AddExpirationToken(new CancellationChangeToken(tokenSource.Token));
            }

I made quite a lot of load tests after the change (even some with much more requests) and I can no longer reproduce the problem so I can guarantee that this fix will work :)))

p.s: What is also bothering me is that the CacheKey objects are static/shared and the FillCacheKey method is changing the elements in the Prefixes collection that is later used in the cache manager. So it is quite possible that if Request1 sets the prefix to Prefix1 and then before being added to the cache (before SetupEntry is called in the cache manager) another request i.e Request2 sets the prefix to Prefix2 (by using different key objects) then the cache entry for Request1 will be added as Prefix2 rather than as Prefix1 in the _prefixes collection of the MemoryCacheManager. This could potentially cause caching issues in the store. Of course now this is hard to be reproduce by a single user playing with the store but when many users browse different pages simultaneously it is quite possible to happen.

I hope you understand me but since its too late now for me you can ask if nothing is clear :)

Thanks,
Boyko

Nop-Templates.com wrote:
Thanks again, Boyko! Here is a work item.

Hi Boyko. I don't quite understand your warring about static CacheKey, after all, they are designed as Properties and not as fields, so new ones should be created every time. I tried to put your concern on the code, please look at it, does it describe a potential problem?

var key1 = NopBlogsCachingDefaults.BlogCommentsNumberCacheKey.FillCacheKey(1, 1, true);
var key2 = NopBlogsCachingDefaults.BlogCommentsNumberCacheKey.FillCacheKey(2, 1, false);

var cacheManager = EngineContext.Current.Resolve<IStaticCacheManager>();

cacheManager.Set(key1, "test FillCacheKey potential problem");
cacheManager.Set(key2, "test FillCacheKey potential problem");

var prefix = NopBlogsCachingDefaults.BlogCommentsNumberPrefixCacheKey.ToCacheKey(2);

cacheManager.RemoveByPrefix(prefix);

var value1 = cacheManager.Get<string>(key1, () => "Deleted");
var value2 = cacheManager.Get<string>(key2, () => "Deleted");

if(value1.Equals(value2, StringComparison.InvariantCultureIgnoreCase))
  throw new Exception("Fail!!!");

If I understood everything correctly, then this code claims that everything works as expected and an exception is not raised.

I understood the problem of bypassing the cycle, although I could not understand the circumstances of its occurrence. But I suggest not changing the loop type, but simply bypassing the copy of the array, could you, please, test the performance of the following implementation of the SetupEntry method:

private void SetupEntry(ICacheEntry entry, CacheKey key)
{
  entry.AbsoluteExpirationRelativeToNow = TimeSpan.FromMinutes(key.CacheTime);
  entry.AddExpirationToken(new CancellationChangeToken(_clearToken.Token));

  foreach (var keyPrefix in key.Prefixes.ToList())
  {
    var tokenSource = _prefixes.GetOrAdd(keyPrefix, new CancellationTokenSource());

    entry.AddExpirationToken(new CancellationChangeToken(tokenSource.Token));
}

I am very grateful for your help in the analysis and testing.