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.

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

Clear Cache With Distributed Cache #5593

Closed
streamzyuk opened this issue Apr 7, 2021 · 13 comments
Closed

Clear Cache With Distributed Cache #5593

streamzyuk opened this issue Apr 7, 2021 · 13 comments

Comments

@streamzyuk
Copy link

nopCommerce version: 4.40.2

Steps to reproduce the problem:
Run Multiple Instance.
Login to Admin Area.
Select Clear Cache.
Check Redis Keys.
Only the key that are set by the instance are cleared.

Distributed cache manager holds a list of keys which are set by the instance running. It uses this list to clear the cache keys from redis. But when reading it does not check the keys list. So understand that this list is used only for clearing the keys.

It may add the keys to the list when TryGetItemAsync returns true but this will not solve the problem since still only a portion of the keys will be stored.

Is there any purpose for it.

I expect clear cache to clear all keys in redis server or cluster.

There is no meaning of clearing the keys set by instance. Since it may not even set anything if the instance is created at later stage. Other instances may already created the cache keys.

@streamzyuk
Copy link
Author

This looks like causing other problems.
Changing settings from the admin area. Since the cache keys are not on the instance, settingsService.ClearCache does not clear anything at all.

@streamzyuk
Copy link
Author

public async Task ClearAsync()
{
//we can't use _perRequestCache.Clear(),
//because HttpContext stores some server data that we should not delete
var appSettings = Singleton.Instance;
var distributedCacheConfig = appSettings.DistributedCacheConfig;
foreach (var redisKey in _keys)
_perRequestCache.Remove(redisKey);

        /*
        using var _ = await _locker.LockAsync();

        foreach (var key in _keys) 
            await _distributedCache.RemoveAsync(key);
        */
        try
        {
            var connectionMultiplexer = ConnectionMultiplexer.Connect(distributedCacheConfig.ConnectionString + ",allowAdmin=true");
            var endpoints = connectionMultiplexer.GetEndPoints(false);
            foreach (var endpoint in endpoints)
            {
                var server = connectionMultiplexer.GetServer(endpoint);

                if (!server.IsSlave)
                {
                    var keys = server.Keys(0,"Nop.*");
                    foreach(var key in keys)
                    {
                        await _distributedCache.RemoveAsync(key);
                    }
                   
                   // await server.FlushAllDatabasesAsync();
                }
            }
            await connectionMultiplexer.CloseAsync(true);
        }
        catch (Exception e) {
            Console.WriteLine(e);
        }
        _keys.Clear();
    }

This is what I have tried. It works but it takes time. For 500 keys in a 6 node standard cluster it takes 458 ms. Basically 1 key deletion = 1 ms.
I tried flushdb. Its faster but with the side effect of deleting the SessionState Keys which is not acceptable.
RemoveByPrefix method may be changed to use the approach above for which have relatively less keys to delete.

But this should be fixed since the current code does not work with keys stored by instances.

@skoshelev
Copy link
Contributor

Unfortunately, we had to add our own list of keys to the DistributedCacheManager to compensate for the lack of methods of the IDistributedCache base interface. But unfortunately, the problem of completely clearing the distributed cache has not been solved yet, we are waiting for the implementation of this ticket and the connection with it of smaller tasks

@streamzyuk
Copy link
Author

Yes I understand that. But the problem is Key List is Per instance which does not solve (actually create a real sync problem with multi instance setup) key management problem in a multi instance setup.

@purplepiranha
Copy link
Contributor

I think this may be what's causing my problem too. If I edit any plugin settings they get saved to the database, but not updated in the cache, even after using the 'Clear Cache' button.

@sinaislam
Copy link

ClearAsync

Does this method solve your problem to remove cache for the Radis?

@purplepiranha
Copy link
Contributor

ClearAsync

Does this method solve your problem to remove cache for the Radis?

No.

I think there are 2 methods causing a problem here within DistributedCacheManager:
ClearAsync() - which should clear the entire cache but doesn't
RemoveByPrefixAsync(string prefix, params object[] prefixParameters) - which is called by ISettingService.ClearCacheAsync() when configuring a plugin. This can't be working because when a plugin's settings are saved and you return to the settings page it has the old values although a check in the database shows that they have changed.

It's also likely that RemoveAsync(CacheKey cacheKey, params object[] cacheKeyParameters) will have the same issue but I haven't looked into that yet.

@antoniodlp
Copy link

What if we store the keys in a a cache entry? Then they would be distributed to all servers.
Can someone tell me what the issue would be with that?

@sinaislam
Copy link

What if we store the keys in a cache entry? Then they would be distributed to all servers. Can someone tell me what the issue would be with that?

First of all, I do not test it myself. But if the below comment is true then it is a severe problem for auto-scaling.

"Yes, I understand that. But the problem is Key List is Per instance which does not solve (actually create a real sync problem with multi-instance setup) key management problem in a multi-instance setup."

If your fixing can resolve this issue then it would be good.

@RomanovM RomanovM modified the milestones: Version 4.50, Version 4.60 Dec 10, 2021
@szilardd
Copy link

szilardd commented Jul 27, 2022

If you are using SQL Server as the data store, this might be a workaround until a better solution is available, by overriding the DistributedCacheManager in a plugin.

using System.Threading.Tasks;
using LinqToDB.Data;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.Caching.Distributed;
using Nop.Core.Caching;
using Nop.Core.Configuration;
using Nop.Data;

namespace Plugin.Misc.MyPlugin.Services
{
    /// <summary>
    /// Overrides base cache manager and enables deleting all keys from database, 
    /// not just the ones that are currently in use. Related github issue:
    /// https://github.com/nopSolutions/nopCommerce/issues/5593
    /// </summary>
    public class MyPluginSqlServerDistributedCacheManager : DistributedCacheManager
    {
        private readonly INopDataProvider _dataProvider;
        private readonly string _tableName;

        public MyPluginSqlServerDistributedCacheManager 
        (
            AppSettings appSettings, 
            IDistributedCache distributedCache, 
            IHttpContextAccessor httpContextAccessor,
            INopDataProvider dataProvider
        ) : base(appSettings, distributedCache, httpContextAccessor)
        {
            _dataProvider = dataProvider;
            _tableName = appSettings.Get<DistributedCacheConfig>().TableName;
        }

        public override async Task ClearAsync()
        {
            // base delete
            await base.ClearAsync();

            // delete from database
            await _dataProvider.ExecuteNonQueryAsync($"TRUNCATE TABLE {_tableName}");
        }

        public override async Task RemoveByPrefixAsync(string prefix, params object[] prefixParameters)
        {
            // base delete
            await base.RemoveByPrefixAsync(prefix, prefixParameters);

            // delete from database
            prefix = PrepareKeyPrefix(prefix, prefixParameters);
            await _dataProvider.ExecuteNonQueryAsync
            (
                $"DELETE FROM {_tableName} WHERE Id LIKE @Prefix + '%'",
                new DataParameter[] { new DataParameter("Prefix", prefix) }
            );
        }
    }
}

And it needs to be registered in INopStartup of the plugin

public class NopStartup : INopStartup
{
    public void ConfigureServices(IServiceCollection services, IConfiguration configuration)
    { 
        var appSettings = Singleton<AppSettings>.Instance;
        var distributedCacheConfig = appSettings.Get<DistributedCacheConfig>();
        if (distributedCacheConfig.Enabled && distributedCacheConfig.DistributedCacheType == DistributedCacheType.SqlServer)
        {
          services.AddScoped<IStaticCacheManager, MyPluginSqlServerDistributedCacheManager>();
        }
    }
}

DistributedCacheManager.ClearAsync and DistributedCacheManager.RemoveByPrefixAsync have to be marked as virtual in nop.

@sinanopcommerce
Copy link

If you are using SQL Server as the data store, this might be a workaround until a better solution is available, by overriding the DistributedCacheManager in a plugin.

using System.Threading.Tasks;
using LinqToDB.Data;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.Caching.Distributed;
using Nop.Core.Caching;
using Nop.Core.Configuration;
using Nop.Data;

namespace Plugin.Misc.MyPlugin.Services
{
    /// <summary>
    /// Overrides base cache manager and enables deleting all keys from database, 
    /// not just the ones that are currently in use. Related github issue:
    /// https://github.com/nopSolutions/nopCommerce/issues/5593
    /// </summary>
    public class MyPluginSqlServerDistributedCacheManager : DistributedCacheManager
    {
        private readonly INopDataProvider _dataProvider;
        private readonly string _tableName;

        public MyPluginSqlServerDistributedCacheManager 
        (
            AppSettings appSettings, 
            IDistributedCache distributedCache, 
            IHttpContextAccessor httpContextAccessor,
            INopDataProvider dataProvider
        ) : base(appSettings, distributedCache, httpContextAccessor)
        {
            _dataProvider = dataProvider;
            _tableName = appSettings.Get<DistributedCacheConfig>().TableName;
        }

        public override async Task ClearAsync()
        {
            // base delete
            await base.ClearAsync();

            // delete from database
            await _dataProvider.ExecuteNonQueryAsync($"TRUNCATE TABLE {_tableName}");
        }

        public override async Task RemoveByPrefixAsync(string prefix, params object[] prefixParameters)
        {
            // base delete
            await base.RemoveByPrefixAsync(prefix, prefixParameters);

            // delete from database
            prefix = PrepareKeyPrefix(prefix, prefixParameters);
            await _dataProvider.ExecuteNonQueryAsync
            (
                $"DELETE FROM {_tableName} WHERE Id LIKE @Prefix + '%'",
                new DataParameter[] { new DataParameter("Prefix", prefix) }
            );
        }
    }
}

And it needs to be registered in INopStartup of the plugin

public class NopStartup : INopStartup
{
    public void ConfigureServices(IServiceCollection services, IConfiguration configuration)
    { 
        var appSettings = Singleton<AppSettings>.Instance;
        var distributedCacheConfig = appSettings.Get<DistributedCacheConfig>();
        if (distributedCacheConfig.Enabled && distributedCacheConfig.DistributedCacheType == DistributedCacheType.SqlServer)
        {
          services.AddScoped<IStaticCacheManager, MyPluginSqlServerDistributedCacheManager>();
        }
    }
}

DistributedCacheManager.ClearAsync and DistributedCacheManager.RemoveByPrefixAsync have to be marked as virtual in nop.

True. But actual issue is the cache is per instance when use the distributed cache.

@skoshelev
Copy link
Contributor

Closed #5593

@AdiIndoSwiss
Copy link

I tried this solution still the cache doesn't load properly in 4.5.01. any suggestions need quickly a working solution. I tried overwriting the distributedcache one but no luck.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants