Expose per operation timeout compatible with CancellationToken

[tylerohlsen]

I would really like the granular control of choosing the timeout per operation. There are many Get operations that I would really like to have a very short timeout, other Gets to have a medium timeout, and Set operations to have a very long timeout. I expected this to be a very common use case.

I use the implementation wrapped from IDistributedCache in the Microsoft.Extensions.Caching.Redis library. I was deceived in thinking per operation timeout was already implemented due to the IDistributedCache implementation exposes overloads that take a CancellationToken. I've recently found out that these tokens are only used to cancel between calls to the StackExchange.Redis library and not passed to StackExchange.Redis at all.

Can we get per operation timeouts implemented? And in a way that either takes a CancellationToken directly or can be adapted from a CancellationToken?

Can this be plumbed in to be ready for Socket to support CancellationToken?

[mgravell]

Hmmm. We could presumably use the state-passing Register overload to good effect here. My main concern would be one of expressing intent: if we assume that commands will usually be written promptly, all we can do is mark the incomplete task as complete (with cancellation): we can't retract the issued command from the server (except for the relatively rare scenario where it hadn't been written yet). So: it is probably safe (in terms of not confusing folks) on idempotent operations like reads. There's also the bigger issue of API change - adding this to every method would be messy. I wonder whether it would pragmatic to use a wrapper instead, i.e. var x = await db.WithCancellation(token).GetAsync(key); Thoughts? (Here WithCancellation would probably return a readonly struct that implements the async API)

…

On Mon, 14 Jan 2019, 16:10 Tyler Ohlsen ***@***.*** wrote: I would really like the granular control of choosing the timeout per operation. There are many Get operations that I would really like to have a very short timeout, other Gets to have a medium timeout, and Set operations to have a very long timeout. I expected this to be a very common use case. I use the implementation wrapped from IDistributedCache in the Microsoft.Extensions.Caching.Redis <https://www.nuget.org/packages/Microsoft.Extensions.Caching.Redis/> library. I was deceived in thinking per operation timeout was already implemented due to the IDistributedCache implementation exposes overloads that take a CancellationToken. I've recently found out that these tokens are only used to cancel between calls to the StackExchange.Redis library and not passed to StackExchange.Redis at all. Can we get per operation timeouts implemented? And in a way that either takes a CancellationToken directly or can be adapted from a CancellationToken? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1039>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABDsPjpgbzwkex2phOE3UDW_fJZV4y3ks5vDKvkgaJpZM4Z-kQH> .

[tylerohlsen]

all we can do is mark the incomplete task as complete (with cancellation)

That would allow the client application flow to continue, which is what is important to me at this time

adding this to every method would be messy

Messy, but consistent. All other Async APIs I've worked with use the overload approach. But I understand that adding to an existing API is more nuanced. I'd leave that up to you.

[mgravell]

it feels like something we should have added in the hard 2.0 break, but: that ship has sailed :(

[tylerohlsen]

All of the existing implementation should not be affected, so I don't think a major rev is necessary. But probably a minor rev.

[mgravell]

agreed, but we can't add an optional parameter without a major rev, and adding an additional overload for each method is ... "hugely undesirable"

[tylerohlsen]

Overloads got a bit more palatable with expression bodied function members.

public Task<RedisValue> StringGetAsync(RedisKey key, CommandFlags flags = CommandFlags.None)
    => StringGetAsync(key, CancellationToken.None, flags);

public Task<RedisValue> StringGetAsync(RedisKey key, CancellationToken token, CommandFlags flags = CommandFlags.None)
{ ... }

But the documentation would still need to be duplicated. And there's a LOT of methods and documentation here.

[tylerohlsen]

Another option would be that WithCancellation(token) could set the token in an internal AsyncLocal ambient context. Then the method signatures would not need to change at all.

[pavdro]

Any update on this? Our team also have a use case for this:)

[toebens]

agreed, but we can't add an optional parameter without a major rev, and adding an additional overload for each method is ... "hugely undesirable"

i would like to see an optional parameter. considering semantic versioning as the method signature changes you are right however its not "really" breaking when still being using it without setting the optional parameter therefor it is not "really" breaking i would say. or am i wrong?

[mgravell]

Yeah, it really is; semver is fine in principle, but when you have a library with lots of consumers, smashing the API on every method isn't really something you want to do very often. As such, just about any other alternative is preferable.

[jjxtra]

So will this break people if they only update this library and not their consuming code? At worse it should compile without changes.

Task<RedisValue> StringGetAsync(RedisKey key, CommandFlags flags = CommandFlags.None, CancellationToken cancelToken = default);

[mgravell]

Indeed. In general, there are two types of breaks:

• breaks that require actual code changes
• breaks that require a recompile of code that references the library, but no code changed

The addition of an optional parameter is in the second category - it isn't a break if you recompile, but: there are a lot of intermediate libraries that reference this library, which means that this is still hugely undesirable - it will lead to unexpected runtime breaks when the packages aren't perfectly aligned. This is a break we usually try hard to avoid, because we don't like breaking other folks' systems.

[jjxtra]

Gotcha, wasn't sure if the .NET runtime was smart enough to link against default parameters without a recompile. That would be nice, but if it's not possible so be it. Personally I would never update to a new package of anything without a full recompile, tests and re-deploy of everything (containers are great for this!)

Thanks for sharing this library by the way, it is very robust!

[mgravell]

Personally I would never update to a new package of anything without a full recompile, tests and re-deploy of everything

In the case of intermediate libraries, that is somewhat outside of your control. I agree that full integration tests would spot this, but I'm also mindful that not everyone would do this. In particular, folks using mocks and unit tests, or that have poor or non-existant automatic testing would still get burned, and I care about those folks too, even if I wish they would have better tests :)