[FEATURE] add `CancellationToken` support to async `IFusionCacheSerializer` methods

[b-c-lucas]

First, thanks for the library; it has been great to see the results during development and while in use in the wild, even if transitioning from my bespoke implementation of a hybrid cache preventing stampedes has proven more difficult than anticipated (but that comes with migrating anything, amirite...).

Problem

The code paths I've had to dig into while investigating issues onboarding to FusionCache have respected plumbed-through CancellationTokens for async methods; however, while investigating an issue after migrating to FusionCache, I noticed that usages of IFusionCacheSerializer were not respecting the passed-in CancellationToken even though the underlying libraries support them (for example, within FusionCacheSystemTextJsonSerializer).

Solution

• Update IFusionCacheSerializer async methods to take a CancellationToken, even if defaulted to default.

public interface IFusionCacheSerializer
{
    /// <summary>
    /// Serialized the specified <paramref name="obj"/> into a byte[].
    /// </summary>
    /// <typeparam name="T">The type of the <paramref name="obj"/> parameter.</typeparam>
    /// <param name="obj"></param>
    /// <returns>The byte[] which represents the serialized <paramref name="obj"/>.</returns>
    ValueTask<byte[]> SerializeAsync<T>(T? obj, CancellationToken cancellationToken = default);

    /// <summary>
    /// Deserialized the specified byte[] <paramref name="data"/> into an object of type <typeparamref name="T"/>.
    /// </summary>
    /// <typeparam name="T">The type of the object to be returned.</typeparam>
    /// <param name="data"></param>
    /// <returns>The deserialized object.</returns>
    ValueTask<T?> DeserializeAsync<T>(byte[] data, CancellationToken cancellationToken = default);
}

• Update implementations to respect the contract update

  • from the given example

    public class FusionCacheSystemTextJsonSerializer : IFusionCacheSerializer
    {
    /// <inheritdoc />
    public async ValueTask<byte[]> SerializeAsync<T>(T? obj, CancellationToken cancellationToken = default)
    {
      using var stream = new MemoryStream();
      await JsonSerializer.SerializeAsync<T?>(stream, obj, _options, cancellationToken);
      return stream.ToArray();
    }
    
    /// <inheritdoc />
    public async ValueTask<T?> DeserializeAsync<T>(byte[] data, CancellationToken cancellationToken = default)
    {
      using var stream = new MemoryStream(data);
      return await JsonSerializer.DeserializeAsync<T>(stream, _options, cancellationToken);
    }
    }

• Pass through CancellationToken from all callers.

  • from the given example, where token already exists

    data = await _serializer.SerializeAsync(distributedEntry, token).ConfigureAwait(false);

[b-c-lucas]

Also, let me know if I can help: the core change should be trivial, and someone like me can easily contribute without too much mess, even though doing so might involve touching more code than I've assumed.

[jodydonetti]

First, thanks for the library; it has been great to see the results during development and while in use in the wild

Thanks, appreciate that!

even if transitioning from my bespoke implementation of a hybrid cache preventing stampedes has proven more difficult than anticipated

If you like to elaborate more, I'm interested! Maybe it can help me ease other transitions.

Problem

The code paths I've had to dig into while investigating issues onboarding to FusionCache have respected plumbed-through CancellationTokens for async methods; however, while investigating an issue after migrating to FusionCache, I noticed that usages of IFusionCacheSerializer were not respecting the passed-in CancellationToken even though the underlying libraries support them (for example, within FusionCacheSystemTextJsonSerializer).

In fact that's strange... and now that you've pointed it out it's quite suprising I've never done that 🤔

Solution

• Update IFusionCacheSerializer async methods to take a CancellationToken, even if defaulted to default. [...]
• Pass through CancellationToken from all callers.

Totally, makes sense.

Let me doublecheck this and will let you know.

Thanks!

[jodydonetti]

All seems good 👍

I'll sleep on this a bit, but will probably include this in next version (v1.4.0) coming out very soon.

[b-c-lucas]

Thanks, @jodydonetti! That turnaround time is impeccable... Should I get used to that? 😉😂

This isn't a blocker for anything I'm doing at the moment, so feel free to release when you're comfortable with it.

To your previous point, do you have a preference for how to communicate feedback for onboarding/migrating existing cache setups to use FusionCache?

Most of my feedback would be to explicitly call out or further accentuate the things that are highly recommended to be configured to avoid pain: for example, certain operations require configuring a specific value and don't fall back to the globally configured log level, so it took a few merges for me to get some logs I would have benefited from out of the gate. There are a couple of other suggestions I might make (or I may benefit from your counsel in figuring out the right approach), but nothing pressing at the moment.

Thanks again for the great solution!

[jodydonetti]

Thanks, @jodydonetti! That turnaround time is impeccable... Should I get used to that? 😉😂

Ahah not really, I mean it depends on the day 😅

To your previous point, do you have a preference for how to communicate feedback for onboarding/migrating existing cache setups to use FusionCache?

I would say a new discussion would be good (and really appreciated!).

In case there are problems, simply opening an issue like this one would be good.

Most of my feedback would be to explicitly call out or further accentuate the things that are highly recommended to be configured to avoid pain: for example, certain operations require configuring a specific value and don't fall back to the globally configured log level, so it took a few merges for me to get some logs I would have benefited from out of the gate.

Interesting, I'd like to know more.

There are a couple of other suggestions I might make (or I may benefit from your counsel in figuring out the right approach), but nothing pressing at the moment.

Good to know 🙂

Thanks again for the great solution!

Thanks to you for chipping in!

[jodydonetti]

Instead, regarding the main thing here meaning cancellation token support for the serializer's async methods, I'd like to better understand this passage:

however, while investigating an issue after migrating to FusionCache, I noticed that usages of IFusionCacheSerializer were not respecting the passed-in CancellationToken even though the underlying libraries support them (for example, within FusionCacheSystemTextJsonSerializer).

I'd like to understand if the issue you were investingating was, in the end, related to the missing cancellation tokens or not and, if so, what was the problem.

I'm asking for a couple of reasons: 1) I'm just curious 😅, to better understand the scenario 2) changing a signature like this is a breaking change, and with some tests I'm doing it smells potentially risky (not sure yet, still investigating) 3) for serialization in general sync is frequently the preferred way. So much so that I recently added an option for that. In particular here, since to talk with IDistributedCache I'm forced to use byte[], async may not be needed at all

Thoughts?

[jodydonetti]

Hi @b-c-lucas , if and when you can, would you let me know about each point above?

It would help me greatly in better understanding the scenario.

Thanks!

[jodydonetti]

Hi, v1.4.0 has been released 🥳

[b-c-lucas]

Hey @jodydonetti - sorry, I missed the ask for clarifications.

The "issue" I was investigating was not directly to FusionCache, but rather very specific to an implementation detail for our usage. I logged the issue as, during my deep diving into various issues during our onboarding/migration, I noticed that the cancellation token was not being forwarded to the register serializer, and I'm personally of a strong opinion that any async code should be a good citizen and at the very least simply forward along a passthrough value if underlying methods called could stand to benefit from doing so. I didn't look closely at the changes made to FusionCache, but I'm surprised to hear adding cancellation token support would be considered a breaking change - providing a default value of default for the CancellationToken parameter is a pretty standard and backward compatible move, but perhaps there was something FusionCache-specific that I didn't consider.

I'll circle back later on a discussion from some of my experiences from the onboarding/migration as you requested.

Cheers, and thanks again for tackling this!

[jodydonetti]

Hi @b-c-lucas

The "issue" I was investigating was not directly to FusionCache, but rather very specific to an implementation detail for our usage.

Good to know!

I logged the issue as, during my deep diving into various issues during our onboarding/migration, I noticed that the cancellation token was not being forwarded to the register serializer, and I'm personally of a strong opinion that any async code should be a good citizen and at the very least simply forward along a passthrough value if underlying methods called could stand to benefit from doing so.

In general, yes. In this particular case, since the input is (currently) always byte[], probably less so. Anyway there is work going on to augment IDistributedCache to also support IBufferWriter & friends, so there's more value to it coming along.

I didn't look closely at the changes made to FusionCache, but I'm surprised to hear adding cancellation token support would be considered a breaking change - providing a default value of default for the CancellationToken parameter is a pretty standard and backward compatible move, but perhaps there was something FusionCache-specific that I didn't consider.

It's not a FusionCache-specific thing, you are thinking about source-compatibility from the consumer side: in that case you are right, that is not a breaking change.

But there are 2 other things to consider, not related to FusionCache but very general:

• binary compatibility: a change like the one I've made breaks binary compatibility
• 3rd party implementations: existing impl of IFusionCacheSerializer will stop compiling (let alone running as-is, see previous point) because the signature has now changed

I decided in this case to go on with it since when updating a package reference they are usually all updated at once (maning, in this case, both FusionCache and the satellite packages like serializers, etc). Also in this case I'm realistically the only one who implemented concrete impl of serializers, so I'm (hopefully) the only one affected.

I'll circle back later on a discussion from some of my experiences from the onboarding/migration as you requested.

Thanks!