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 <notifications@github.com 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.

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

[kikaragyozov]

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.

[KrzysztofBranicki]

@mgravell any update on this issue? My team also stumbled on this use-case. Maybe it's good reason to release 3.0 with APIs aligned to async conventions.

[NickCraver]

For anyone still wanting to take the overhead here per-command, .NET 6 adds a myTask.WaitAsync(TimeSpan) you can use for this purpose (or on any Task). Rather than add this to the library breaking all consumers, I'd recommend you use that new extension in .NET.

[jjxtra]

A great stop gap solution for sure. For the next major version where breaking changes are ok it would be nice to see a cancel token with default value of default added.

[NickCraver]

Just to set expectations here, just because a major version can have breaking changes doesn't mean we should make breaking changes, especially to all async methods. There are no plans for 3.x yet so we'll revisit closer to, but I wanted to chime in on what bar we set for not breaking people. For as many people as reasonably possible, upgrades should be non-breaking (major or not).

[attique333]

I am using redis cach, in task.Delay in am passing cancellation token and set in redis cache in second operation i am getting that cache and want to cancel that cancellation token which is not actually cancelling token…. But same this is prefectly working with inmemory cache

[NickCraver]

@attique333 I'm can't follow your scenario here - do you have a code example to advise on? We don' take any of those in the library, so not sure where you're touching StackExchange.Redis code here to help suggestions on things to do.

[attiqueahmed333]

@NickCraver My scenario was to create cancellation token and save in the memory either in-memory or redis cache and Task.Delay(20 Sec) to stop and wait...... and on the other side i am getting that cancellation token from either in-memory or redis cache and perform some operation and after finishing i am calling back that previous cancellation token which is in Task.Deplay for 20 Sec to cancel operation and it normally goes to systemoperationexception which is perfect working in in-memory cache but for the redis cache it is not working as calling back to terminate cancellation token not terminating and not giving me any systemoperationexception. So after work around i found pub sub event of redis exchange.....So when i completed operation then i am publishing cancelation token and in Task.Delay 20 sec i am subscribing and inside i am cancelling token and their it is giving me systemoperationexception, so its working perfectly

[ArthNRick]

Só para definir expectativas aqui, só porque uma versão principal pode ter alterações de quebra não significa que devemos fazer alterações de quebra, especialmente em todos os métodos assíncronos. Ainda não há planos para o 3.x, então vamos revisitar mais perto, mas eu queria saber qual barra definimos para não quebrar as pessoas. Para o maior número de pessoas razoavelmente possível, as atualizações devem ser ininterruptas (principais ou não).

I understand your point, but normally cancellation tokens are optional parameter, I don't see a problem in this case as it wouldn't break using a default value.

[NickCraver]

@ArthNRick This is a common misconception in .NET, that adding an optional parameter isn't a breaking change, but it is. The default value isn't stored on the method it belongs to when you compile, it's stored on the caller. Methods are always called with all their arguments. Any library compiled on top of us would throw missing method exceptions until they recompiled from source and re-released - that's a binary breaking change.

I think it helps to demonstrate this especially given this issue, so here's a demo: https://sharplab.io/#v2:CYLg1APgAgTAjAWAFBQMwAJboMLoN7LpGYZQAs6AsgBQCU+hxTlApgC4AWA9sAOoCWnAPIAHNvy4A7AIYAbAArSATtIC21AEQDOAQUUrVG2gG5GTIq049tHUeKlz9ausfQB6N+htcArm3TS6CLKamboAL5hYWiYcAAMVOzcfIK2YhIyCiHqUPHoAM7oALzoGgDKXKosACIsAGbSPrJsRsUAfOiSTbKmSOFAA

[ArthNRick]

@ArthNRick This is a common misconception in .NET, that adding an optional parameter isn't a breaking change, but it is. The default value isn't stored on the method it belongs to when you compile, it's stored on the caller. Methods are always called with all their arguments. Any library compiled on top of us would throw missing method exceptions until they recompiled from source and re-released - that's a binary breaking change.

I think it helps to demonstrate this especially given this issue, so here's a demo: https://sharplab.io/#v2:CYLg1APgAgTAjAWAFBQMwAJboMLoN7LpGYZQAs6AsgBQCU+hxTlApgC4AWA9sAOoCWnAPIAHNvy4A7AIYAbAArSATtIC21AEQDOAQUUrVG2gG5GTIq049tHUeKlz9ausfQB6N+htcArm3TS6CLKamboAL5hYWiYcAAMVOzcfIK2YhIyCiHqUPHoAM7oALzoGgDKXKosACIsAGbSPrJsRsUAfOiSTbKmSOFAA

Your point makes sense, up to a point. I understand how it works, but most deployment scenarios require a new build. it doesn't make much sense for someone to update a library to a new version available in the nuget just by downloading the new dll and copying the file to the productive environment, the normal and acceptable way is that the nuget is updated by the tools and a new build is done, if there is a breach of contract, there will be a break in the build, and corrections can be made. I can't imagine someone updating this library without doing a new build of its code.

[jjxtra]

@ArthNRick This is a common misconception in .NET, that adding an optional parameter isn't a breaking change, but it is. The default value isn't stored on the method it belongs to when you compile, it's stored on the caller. Methods are always called with all their arguments. Any library compiled on top of us would throw missing method exceptions until they recompiled from source and re-released - that's a binary breaking change. I think it helps to demonstrate this especially given this issue, so here's a demo: https://sharplab.io/#v2:CYLg1APgAgTAjAWAFBQMwAJboMLoN7LpGYZQAs6AsgBQCU+hxTlApgC4AWA9sAOoCWnAPIAHNvy4A7AIYAbAArSATtIC21AEQDOAQUUrVG2gG5GTIq049tHUeKlz9ausfQB6N+htcArm3TS6CLKamboAL5hYWiYcAAMVOzcfIK2YhIyCiHqUPHoAM7oALzoGgDKXKosACIsAGbSPrJsRsUAfOiSTbKmSOFAA

Your point makes sense, up to a point. I understand how it works, but most deployment scenarios require a new build. it doesn't make much sense for someone to update a library to a new version available in the nuget just by downloading the new dll and copying the file to the productive environment, the normal and acceptable way is that the nuget is updated by the tools and a new build is done, if there is a breach of contract, there will be a break in the build, and corrections can be made. I can't imagine someone updating this library without doing a new build of its code.

1000%. It's madness to deploy to production without doing a full rebuild, unit and integration tests.

[NickCraver]

I hear y'all, but you're missing the scenarios. You can depend on library A. Library A depends on us (we're library B). If you upgrade B and it breaks everything A uses, your build will not fail.

You do not rebuild your dependencies when you build. That's a binary breaking change. And the way transitive NuGet references work, there can be mutually exclusive working subsets of libraries, e.g. another library C which is on us and upgrades, making it so A and C can't work until both are updated.

With a quarter billion downloads, these are things we take very, very seriously. Doing this would break a lot of people.

[jjxtra]

That's what the unit and integration tests are supposed to catch. Anyone not doing this with a proper staging environment is going to deal with production issues of all kinds.

[mgravell]

No level of integration tests will help you, though; some of the things that take dependencies on us are big and move with slower release velocity (think things like "asp.net core", which ships with the .net runtime which ships a new version yearly, with periodic service releases of existing runtimes).

If we break an API like this, then: sure, your integration test will flag that it doesn't work, but you as a consumer can't do anything to fix it. You either need to revert the SE.Redis update and live without updates, or wait for (and deploy) the corresponding runtime update. Or similarly for "whatever 3rd party lib/libs" - you need to find their release cycle (or the intersection of release cycles if multiple libs are involved), and coordinate with those. Assuming they're still maintained.

This gets worse if you're also a library author, as now you've impacted your own consumers and not just yourself.

Short version: we're not going to do that to people, because we're not jerks (at least, not full time).

(also, by coincidence I'm the person at MSFT who will get the angry tickets about "asp.net redis cache isn't working any more, fix it fix it fix it fix it fix it", and I don't want to do that to myself either)

[ArthNRick]

if they are not comfortable updating signatures to not break legacy applications, new signatures that accept cancellation tokens could be created. I believe it is not ideal to have asynchronous methods that cannot be canceled naturally, having to use workarounds to be able to proceed with the execution if any condition prevents you from continuing to wait.