Stackexchange.redis: 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 <[email protected] 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
https://github.com/StackExchange/StackExchange.Redis/issues/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.

```c#
public Task StringGetAsync(RedisKey key, CommandFlags flags = CommandFlags.None)
=> StringGetAsync(key, CancellationToken.None, flags);

public Task 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 :)

[SpiritBob]

Correct me if I'm wrong, but for those that need this functionality right at this moment, wouldn't it be okay for them to implement a Task extension method for a timeout? (using Task.Delay that accepts the cancellation token in question, and some predefined by you timeout time)

If the delay task finishes/is cancelled before the Task with the redis operation, you insert your custom logic to handle that scenario?

I'm aware that at a much lower level involving sockets and network IO the operation of the previous task would still be in motion, but if your API really needs this, isn't this a viable approach?

[edokan]

@jjxtra

Gotcha, wasn't sure if the .NET runtime was smart enough to link against default parameters without a recompile.

default values for arguments are compiled into the caller, so even if they change default value for the argument you would still need to recompile. I tried that for a demo for my colleagues with net 4.x, not sure about core versions.