Support async correctly

[tejuguasu]

Async/Sync methods

• Add "Async" suffix to the async methods and update references. These changes were done to the ICache interface and all classes implementing it.
• Create the Sync version of the renamed methods.
• Introduce GetSetLocked method to the CacheEntry class to reduce code duplication between GetSetAsync and GetSet implementations.

Tests

• Rename tests to include async when the async methods are tested.
• Add sync version of each test but those checking concurrence and multithreading behavior.

Not related to async:

• In RedisCache, rename private properties InvalidationChannel and GuidLength to start with an underscore and lower case.
• Remove unnecessary using from IRedisValueSerializer.

[sklivvz]

I propose adding a Parent property that the CompositeCache sets to itself in the secondary cache. Then the handler for a network event should check this and pass on the message if it is set instead of handling it. This allows an event coming from the network to "bubble up" all the way to the topmost parent which can then delegate correctly. Also the Invalidate() method should have a propagateToNetwork bool (default true) that is passed down to avoid unnecessary calls. So:

Interface signature

• GetSet and GetSetAsync -- unchanged
• Invalidate(bool propagate = true) and InvalidateAsync(bool propagate = true)
• ICache Parent {get; set;}

[aaubry]

What's the use-case for propagate = false ?

[sklivvz]

What's the use-case for propagate = false ?

In case of multiple layers of distributed caching we need to keep track of whether we need to notify the network or not (I think only the bottom layer should)

[aaubry]

In case of multiple layers of distributed caching we need to keep track of whether we need to notify the network or not (I think only the bottom layer should)

This makes sense, although I don't see how a composite cache will know whether it is at the bottom of the chain to pass that parameter. How would that work ?

[aaubry]

I propose adding a Parent property that the CompositeCache sets to itself in the secondary cache. Then the handler for a network event should check this and pass on the message if it is set instead of handling it. This allows an event coming from the network to "bubble up" all the way to the topmost parent which can then delegate correctly. Also the Invalidate() method should have a propagateToNetwork bool (default true) that is passed down to avoid unnecessary calls. So:

Interface signature

* `GetSet` and `GetSetAsync` -- unchanged

* `Invalidate(bool propagate = true)` and `InvalidateAsync(bool propagate = true)`

* `ICache Parent {get; set;}`

I think we need a different method to bubble up the invalidations. Otherwise, there is no context and you can't know whether the invalidation needs to be propagated up or be executed.

[sklivvz]

After some more thought between me and @aaubry here's the current proposal.

Interface has Invalidate and GetSet (with async variants). No propagate parameter; no Parent property.

RedisCache does not invalidate across the network (just storage)

A new cache is added that only sends and receives invalidation messages from the network, and invalidates its only child cache - it's a passthrough for the rest.

This allows building caches like this new NetworkCache(new RedisCache) and we get a RedisCache standalone that also invalidates; new NetworkCache(new MemoryCache) gives us a new kind of Memory cache that can be invalidated across the network; new NetworkCache(new CompositeCache(new MemoryCache, new RedisCache)) gives a new kind of composite cache with the correct network invalidation semantics

[aaubry]

It seems that a design that would work would be to extract the pubsub behaviour to a different ICache implementation, which would be used as a decorator to the top-level cache. Something like this:

class Xxx : ICache // Naming is left as an exercise to the reader
{
    private readonly ICache _inner;

    public Xxx(ICache inner)
    {
        this._inner = inner;
    }

    public void Invalidate(string key, bool propagate = true)
    {
        _inner.Invalidate(key, propagate);
        BroadcastInvalidation(key); // Will send the invalidation to all other instances
    }

    private void OnInvalidation(string key) // Called when an invalidation is received from the network
    {
        _inner.Invalidate(key, propagate: false);
    }

    // GetSet members would be pass-through
}

This approach separates the cache and synchronization concerns and removes the need of the Parent property altogether. Does this make sense ?

[sklivvz]

I think propagate is not necessary anymore, and that OnInvalidation should know whether to call the async or sync variant

[sklivvz]

also I think there's no need to call invalidate on invalidate, because there will be a network pulse

[aaubry]

I think propagate is not necessary anymore, and that OnInvalidation should know whether to call the async or sync variant

I think you still need the propagate parameter, otherwise every instance of RedisCache will attempt to delete the same key, while only one call is necessary.

My example was pseudo-code, it's missing the async variants. In the case of the OnInvalidation, actually it should be free to call either the sync or async variant, since this is happeing in a some background task or thread.

[aaubry]

also I think there's no need to call invalidate on invalidate, because there will be a network pulse

If you don't call invalidate, then the caller won't see the invalidation immediately, and also, assuming that the "propagate" parameter is kept, you need to pass it to true in that case to make sure that you only invalidate Redis (or any other shared cache) once.

[sklivvz]

I think you still need the propagate parameter, otherwise, every instance of RedisCache will attempt to delete the same key, while only one call is necessary.

You are making a good point, I think it's correct. I hate concurrent programming :-/

[aaubry]

Naming and caching - the hardest problems in software development.