Skip to content
This repository has been archived by the owner on Apr 27, 2019. It is now read-only.

Accept CancellationToken in higher-level APIs #55

Closed
breischl opened this issue May 24, 2016 · 6 comments
Closed

Accept CancellationToken in higher-level APIs #55

breischl opened this issue May 24, 2016 · 6 comments

Comments

@breischl
Copy link

The higher-level APIs (eg, ConsulClient.Catalog.Service()) do not accept CancellationToken arguments. It would be nice if they did.

It appears that the methods they call into (eg, Consult.GetRequest<T>.Execute()) do accept a CancellationToken, so it should just be a matter of adding method overloads and passing the argument through.

@highlyunavailable
Copy link
Contributor

highlyunavailable commented May 24, 2016

Thanks for the report! I think this is an overly specific interpretation on my part of the spec which says:

The nodes and services endpoints support blocking queries and tunable consistency modes.

But now that I think about it, I don't see why the node and service (singular) endpoints shouldn't support blocking queries. Some of the endpoints do not (e.g. datacenters) and thus it doesn't make sense to have a CancellationToken for Datacenters() but for node and service it absolutely does if they support blocking queries.

You are correct, it would be extremely simple to add them by adding another overload to the method - see services vs service. I'll fix it up tonight, or if you want, I'm happy to accept a PR with a fix.

@breischl
Copy link
Author

I tend to think they should all support CancellationTokens. That would allow clients to protect from failure cases like ridiculous network latency. And I don't see any particular harm that would come from allowing most operations to be cancellable at that level.

@highlyunavailable
Copy link
Contributor

The only reason I put in CancellationTokens in the signatures in the first place was to allow long polls (watches) to be aborted, but I see your point. Generally you're accessing 127.0.0.1 with this API so if you've got network latency to localhost something else is very wrong, but it's a valid use case and I don't see a reason not to support cancelling all requests.

@breischl
Copy link
Author

I'm actually planning on using it to monitor changes from a machine that will (probably) not be running a Consul Agent locally. But I might just be weird. :)

@highlyunavailable
Copy link
Contributor

As I said, a perfectly valid use case!

@highlyunavailable
Copy link
Contributor

Due to the amount of work required to add 1.5x the amount of methods in every existing interface/source file, I'm going to push work on making all calls abortable to another issue and target making all calls abortable with 0.7.0 unless you have a pressing need (e.g. you're running into problems where your service is failing because you can't abort a non-longpoll call). The release 0.6.4.4 should resolve this specific issue and add the two missing overrides though, and the package is uploaded to Nuget. Please don't hesitate to let me know if you have any further issues.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants